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.

Sign up for GitHub

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

Jump to bottom

Simplify unit tests runner #4087

Merged
merged 4 commits into from
May 5, 2021
Merged

Simplify unit tests runner #4087

merged 4 commits into from
May 5, 2021

Conversation

ethervoid
Copy link
Contributor

Signed-off-by: Mario de Frutos mario@defrutos.org

What this PR does:

This PR simplifies how the test tool works reducing significantly the unit test time

Which issue(s) this PR fixes:
Fixes #3579
Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@ethervoid
WIP
742ad6c 
Signed-off-by: Mario de Frutos <mario@defrutos.org>
@ethervoid
Copy link
Contributor Author

@pracucci I this the approach you had in mind for this issue?

pracucci
pracucci reviewed Apr 19, 2021
View reviewed changes
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks for proposing this! The major slowdown is introduced by -race -covermode=atomic which has been removed in this PR. However, removing -race also lowers the confidence we get in unit tests so I'm not sure we should remove it.

@pracucci
Copy link
Contributor

@pracucci I this the approach you had in mind for this issue?

The idea in the issue was completely removing the tools/test and just running go test <cli flags> in Makefile, while keeping the same CLI flags (except for the coverage which is something I believe we don't really need in CI).

@ethervoid
Copy link
Contributor Author

The idea in the issue was completely removing the tools/test and just running go test in Makefile, while keeping the same CLI flags (except for the coverage which is something I believe we don't really need in CI).

I see, didn't want to be aggressive removing the file, mainly because of the coverage part. I'll remove that test tool and just use the plain old test CLI.

THx!

@ethervoid
Simplify the test targets
f5e1b6e 
- Remove the complex tools/test script in favor of simplicity
- Created test and cover tasks in the Makefile that will call go test
  directly

Signed-off-by: Mario de Frutos <mario@defrutos.org>
@ethervoid
Copy link
Contributor Author

@pracucci I've made the changes:

  • Removed the /tools/test script
  • Modified the test task in the Makefile
  • Added a new cover task to the Makefile just in case is needed by someone in the future

I've checked and now is taking 10 minutes when the old script was taking 24 minutes. Mainly because we've removed the coverage part:

New

Screenshot from 2021-04-30 12-53-47

Old

Screenshot from 2021-04-30 12-53-42

@ethervoid ethervoid marked this pull request as ready for review April 30, 2021 11:07
pracucci
pracucci reviewed Apr 30, 2021
View reviewed changes
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM (modulo a fix). Thanks!

Makefile Show resolved Hide resolved
ethervoid and others added 2 commits April 30, 2021 16:57
@ethervoid @pracucci
Update Makefile
6a1d7ec 
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Mario de Frutos <mario@defrutos.org>
@ethervoid
Remove covermode in test target for the makefile
8564d53 
Signed-off-by: Mario de Frutos <mario@defrutos.org>
pracucci
pracucci approved these changes May 4, 2021
View reviewed changes
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

pstibrany
pstibrany approved these changes May 5, 2021
View reviewed changes
Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thank you!

@pracucci pracucci merged commit ba93ac0 into cortexproject:master May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pstibrany pstibrany pstibrany approved these changes

@pracucci pracucci pracucci approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Simplify unit tests runner
3 participants
@ethervoid @pracucci @pstibrany