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

feat(*): add --name, --exclude-names-file and --log-success options #721

Merged
merged 3 commits into from
Nov 6, 2023

Conversation

hanshuebner
Copy link
Contributor

These two options are meant to be used together to allow re-running only the failing tests of a test suite. The --log-success option appends the name of each test to the given file, the --filter-out-file option specifies a file that lists tests which should not be run.

@hanshuebner
Copy link
Contributor Author

This needs a test and the getFullName function needs to be factored out, but what do you think about the approach @Tieske @hishamhm ?

@alerque
Copy link
Member

alerque commented May 12, 2023

Wouldn't the more typical pattern be to log failures and specifically rerun only previously failed tests? Kinda brainstorming here but it seems like that's a pattern I've used before in other test frameworks.

@hanshuebner
Copy link
Contributor Author

@alerque The thing with a "failed" list is that it is harder to maintain because entries need to be removed from it once they pass, while the "succeeded" list can just be appended to. In fact, the --filter-out-file and the --log-success file can be the same for every (re-)try of the test suite. I am aware that ideally, you'd want to fix your tests so that they are not flaky, but we are in a situation where we cannot quickly do that. Thus, being able to run a large test suite repeatedly until all flaky tests have passed is what we need.

I can add a --filter-file and a --log-failure option if you think that someone would find that more useful. The complication is that one would have to specify --filter-file only in the second and subsequent runs and use the --log-failure file of the previous run (if any) as input to --filter-file. I'd prefer the simple solution, even if it might be unconventional. Let me know what you think.

@alerque
Copy link
Member

alerque commented May 12, 2023

I assumed the use-case for this was hacking on the code until the tests pass. If dealing with flaky tests is the use case would a --retry-failed or similar way to give failed tests a second (or more) go in one test runner help?

@hanshuebner
Copy link
Contributor Author

A --retry-failed option would work in principle, but that would make it less apparent that there are flaky tests. We want to be aware of the flakiness, so a failure of the test suite run is good and a manual intervention is acceptable. What we want to avoid is having to re-run all the tests that passed, as that greatly reduces our productivity. With my proposed approach, we can reduce the time that it takes to re-run a single failed test from 20-30 minutes to under one minute, which is a substantial win.

@hanshuebner hanshuebner changed the title feat(*): add --filter-out-file and --log-success options feat(*): add --exclude-names-file and --log-success options May 15, 2023
@hanshuebner hanshuebner changed the title feat(*): add --exclude-names-file and --log-success options feat(*): add --name, --exclude-names-file and --log-success options May 15, 2023
spec/cl_spec.lua Outdated Show resolved Hide resolved
Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

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

Fixup lint error by using variable naming convention for unused vars.

@hanshuebner hanshuebner requested a review from alerque June 26, 2023 07:57
@hanshuebner hanshuebner force-pushed the rerun-failed branch 2 times, most recently from 92b54fe to 570f611 Compare June 26, 2023 08:04
@hanshuebner hanshuebner force-pushed the rerun-failed branch 2 times, most recently from f02606d to e4bc2f4 Compare November 6, 2023 10:20
@hanshuebner
Copy link
Contributor Author

@alerque Is this PR now mergeable? We would like to start using the added functionality more and it would be good to have it be in the upstream repo for that reason.

These two options are meant to be used together to allow re-running only
the failing tests of a test suite.  The --log-success option appends the
name of each test to the given file, the --exclude-name-file option
specifies a file that lists tests which should not be run.
completions/bash/busted.bash Show resolved Hide resolved
try Show resolved Hide resolved
busted/runner.lua Outdated Show resolved Hide resolved
@alerque
Copy link
Member

alerque commented Nov 6, 2023

I actually just fixed all the nit picks from my code review, but I can't push it because apparently you don't have the "allow maintainers" option checked in this PR.

@alerque
Copy link
Member

alerque commented Nov 6, 2023

Can you either allow me access or pull the commits from this branch or split up the commits yourself as in that branch?

@hanshuebner
Copy link
Contributor Author

Thank you for your review - I've addressed your comments. Where would I enable the "allow maintainers" option for next time?

@alerque
Copy link
Member

alerque commented Nov 6, 2023

There should be a check box on the right sidebar in this PR I think...

Allow edits and access to secrets by maintainers

@hanshuebner
Copy link
Contributor Author

I don't see the options, maybe because I'm not an org administrator of Kong? Anyway, is this OK now or do you rather want me to use your branch instead?

@alerque
Copy link
Member

alerque commented Nov 6, 2023

Thank you for your review - I've addressed your comments. Where would I enable the "allow maintainers" option for next time?

Unfortunately you can't address a comment about not touching irrelevant code by re-touching it again in another commit. That will just turn up 2 irrelevant commits in history on blame/bisect etc. I edited the history in way to completely remove the style change and move the unrelated bit to a different commit.

Yes you might be right about not being able to allow me access to an org you don't administer.

I see three options to fix this now:

  1. You can pull in my branch linked above by adding a remote, then git reset --hard <branch_name_or_sha>, then force push this so that you send my commits to this PR.

  2. You do the same git revise and/or git rebase foo to split and edit existing commits.

  3. I can merge the commits manually outside of GitHub. You'll still be credited with the commit authorship but thy PR will be marked as closed instead of as merged. Up to you if you care.

No sweat off my back for any of the above, they just have tradoffs for you ;-)

@hanshuebner
Copy link
Contributor Author

I've reset the PR to your branch and force-pushed. Thanks!

Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

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

Looks good to me now, and sorry for the bother. Thanks for the contribution too, I'll personally get some use out of this feature set!

@alerque alerque merged commit c4093a3 into lunarmodules:master Nov 6, 2023
26 checks passed
@hanshuebner hanshuebner deleted the rerun-failed branch November 6, 2023 11:30
@hanshuebner
Copy link
Contributor Author

Thank you for merging, can you prepare a release as well or can I ping someone else to do it?

@alerque
Copy link
Member

alerque commented Nov 6, 2023

See v2.2.0.

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

Successfully merging this pull request may close these issues.

2 participants