-
Notifications
You must be signed in to change notification settings - Fork 40
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
chore: Move testing to top-level tests
folder
#446
chore: Move testing to top-level tests
folder
#446
Conversation
Should the |
Personally I saw it as a "subtype of integration testing", therefore belonging to the Any reason for it to be standalone? |
I saw conversely, where I agree there may be other kinds of benchmarks, but they can all live under |
Either way is fine. |
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results... |
This broke the integration with the bot. Looking into it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate this re-structuring work for moving tests in one place @orpheuslummis, I have the following feedback:
- suggestion: Rename of the PR to something like:
chore: Move testing to the top-level ./tests folder
(mainly only concerned about the usage of the wordLocating
. - todo: You missed updating the
.gitignore
file where we ignore these:bench/*.log
bench/*.svg
(check the commit below)
Orpheus: This broke the integration with the bot. Looking into it
I have fixed the issue of your branch in another branch that I branched off of your branch : lone/chore/integration-tests-location, more notably just the last commit: 1ea24ed
I tested it by opening this dummy PR: #453 and it works.
The problem was that when you run go benchmarks it outputs the full path and groups your bench outputs related to the full path of the package. I have added a step where we change it to just the package name rather than full location + package name. This should also ensure if we move things around in the future that we won't break it again as the benchstat expects matching package names in the stored artifact report (different path on develop) and the new report (generated in the new PR).
You can just rebase the branch or just cherry pick that commit and it should do the .gitignore
suggestion and fix the broken comparison bot issue (or do it manually, whichever one you prefer).
Thanks @shahzadlone !! |
tests
folder
Excuse my editor's auto-formatting of YAML files |
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
Dismissing was the only way to "accept"
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
RELEVANT ISSUE(S)
Resolves #445
DESCRIPTION
testing
bench
anddb/tests
(renamed todb
) in itSee issue for motivation.
HOW HAS THIS BEEN TESTED?
Made sure tests run by running
make test
andmake test:bench
.CHECKLIST:
ENVIRONMENT / OS THIS WAS TESTED ON?
Please specify which of the following was this tested on (remove or add your own):