-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
ci: Find duplicate and slow tests #9188
Conversation
Thanks for opening this pull request!
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## alpha #9188 +/- ##
==========================================
- Coverage 94.16% 94.14% -0.02%
==========================================
Files 186 186
Lines 14751 14751
==========================================
- Hits 13890 13888 -2
- Misses 861 863 +2 ☔ View full report in Codecov by Sentry. |
It still logs duplicate tests:
|
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.
Some minor log improvements
Signed-off-by: Manuel <5673677+mtrezza@users.noreply.github.com>
Signed-off-by: Manuel <5673677+mtrezza@users.noreply.github.com>
Signed-off-by: Manuel <5673677+mtrezza@users.noreply.github.com>
Signed-off-by: Manuel <5673677+mtrezza@users.noreply.github.com>
Signed-off-by: Manuel <5673677+mtrezza@users.noreply.github.com>
@mtrezza I removed all the dups, looks like there around 75 tests that take longer than a 1000ms. This is ready for review. |
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.
The clean-up and optimization done with this PR is great. But the changes in the test names can be problematic going forward.
I understand that a test needs a unique ID to reference the test. But this also means the title suddenly is not just a test description but has a function that influences this logic. It requires manual data duplication, i.e. add the DB name and version to the text, even though this is defined in the test methods used.
Maintenance: If there are duplicate tests, the CI passes, if I'm not mistaken. That means neither contributors nor reviewers can consider the result of this check. In the end, we'll accumulate test duplicates until someone looks at the logs and cleans it up.
Review effort: This adds an additional review step to verify that the test title includes the methods that are used for the test, DB type and version.
Error prone: New tests are often created by copy/paste/modify. When changing the method, the text description now also has to adapt accordingly, including any specified DB and version. If a test description includes an incorrect MongoDB version, the failing test's title in the logs is misleading.
I'm thinking about how this could be addressed...
Worst case, we throw out the test duplication detection until we found a solution and keep the - I believe by itself already very valuable - execution time analysis.
We may be able to uniquely identify a test by its name, file and code line number without relying on a unique test description. We are also introducing test IDs for the external Oracle DB adapter. A solution could be to add IDs to all tests, with VSC and regex scripting maybe not much of an effort.
@mtrezza I reverted the DB specific duplicate tests. They can be handled in a separate PR. |
🎉 This change has been released in version 7.3.0-alpha.2 |
🎉 This change has been released in version 7.3.0-beta.1 |
🎉 This change has been released in version 7.3.0 |
Pull Request
Issue
With over 3000+ tests and counting, over the years the repository has gone from 15+ minutes of CI time down to under 6 minutes with community efforts #7262. I had the idea years ago to find slow tests and duplicate tests with the same name and functionality using a custom Jasmine Spec Reporter.
This can also help find bottlenecks in performance or tests improvements for instance
This takes 3 seconds and I don't know why maybe it's internal or a Jasmine best practice thats missing.
Optimizing tests as they are created is the goal.
Approach
setTimeout
it('supports bytes range if start and end undefined)
just to make sure it displays in CISub 5 minutes