Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SmartFlank: Use average test time for new tests when possible #979

Closed
wants to merge 1 commit into from
Closed

SmartFlank: Use average test time for new tests when possible #979

wants to merge 1 commit into from

Conversation

CristianGM
Copy link
Contributor

Fixes #977

Test Plan

How do we know the code works?
I've modified the previous tests to show it's behavior change

Checklist

  • Documented
  • Unit tested
  • release_notes.md updated

Copy link
Contributor

@jan-goral jan-goral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, great thanks for your contribution! This improvement looks really smart. We have to consider if we can merge it as is or integrate as an additional option because it is changing default behavior a little.

@bootstraponline #635 (comment) still we don't want an option for it?

@CristianGM
Copy link
Contributor Author

Adding a flag for average would be nice, and IMHO adding a parameter for the default was a must.
I understand trying to limit the number of parameters but it seems the difference on what's the "default" for different devs is too big to just say 10s or 120s.

I'm assuming here that for a project all the tests are kind of the same (for sure there will be a variance, but I don't expect it to be minutes). Having a slightly bigger shard shouldn't be an issue, even smaller is fine if it's not under 1 minute so we have the $$$ issue (just paying for unused seconds, no drama but it's still $).

@bootstraponline
Copy link
Contributor

@bootstraponline #635 (comment) still we don't want an option for it?

@jan-gogo Thanks for this comment Jan! I think you're correct we should have an option. I hoped that we could select one value for all the users, however an option makes the most sense.

@bootstraponline
Copy link
Contributor

Adding a flag for average would be nice, and IMHO adding a parameter for the default was a must.

Agreed that we should have a parameter for the default. Thanks for sharing your use case so we can improve Flank!

@CristianGM
Copy link
Contributor Author

So...
Parameter for the default?
boolean parameter for using average "prediction"?

Do you want me to do the changes? or is it something you want to do? (first time i look at Flank code, but I guess adding the parameter won't be too complex, naming may be)

@bootstraponline
Copy link
Contributor

Do you want me to do the changes? or is it something you want to do? (first time i look at Flank code, but I guess adding the parameter won't be too complex, naming may be)

I think it'll be faster to have the team do the changes. We'll discuss the best way to implement this.

@bootstraponline
Copy link
Contributor

I'm closing this pull request for now. We discussed as a team and prioritized the ticket in the ranked column on ZenHub. There are a few higher priority tasks that'll be worked on first (such as fixing the rate limit issue).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SmartFlank default test time is too big
3 participants