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

Support for multiple "only" #922

Closed
tpburch opened this issue May 22, 2019 · 5 comments
Closed

Support for multiple "only" #922

tpburch opened this issue May 22, 2019 · 5 comments
Assignees
Labels
bug
Milestone

Comments

@tpburch
Copy link

@tpburch tpburch commented May 22, 2019

There is some odd behavior around only flags.

I have found in recent versions of lab (>= 18 - maybe earlier), that I can put only on multiple tests and they will all run. My assumption was that this was an undocumented new feature. However, I also found that this does not work when the elements marked only share a common experiment that is > 2 levels higher. This is due to the implementation of internals.skipAllButOnly in runner.js.

So, I was planning to submit a PR that allows multiple only flags regardless of depth. Then I came across the test:

it('reports an error if there is more than one "only", even accross multiple scripts', async () => {

According to this test, multiple only flags should always throw an error. This test passes, but it passes because the assertions are being skipped:

        try {
            await Lab.execute([script1, script2], {}, null);
        }
        catch (ex) {
            expect(ex.message).to.contain([
                'Multiple tests are marked as "only":',
                'Test: test a',
                'Experiment: test2'
            ]);
        }

Based on the current implementation, this test passes because the execute doesn't reject, so nothing gets caught. If I add a Code.fail() or throw new Error() at the end of the try block, the test then fails.

So, what I'd like is for lab to support multiple only flags - and run every test/experiment that is flagged with only. If this seems reasonable, I can make a small PR to implement this behavior.

If not, then there is likely a bug to be filed/fixed, since multiple only flags are currently respected in some cases, but not others.

Thoughts?

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented May 22, 2019

@tpburch thank you for raising this. Your expectation is the same as mine, every only will execute. This particular test case looks like a mistake and should be removed or fixed.

@tpburch

This comment has been minimized.

Copy link
Author

@tpburch tpburch commented May 22, 2019

@geek that being the case, multiple onlys doesn't completely work right now. I could work up a PR that would include removing the erroneous test, if that seems reasonable?

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented May 23, 2019

@tpburch that will be fantastic if you can create a PR to fix the current behavior.

@tpburch

This comment has been minimized.

Copy link
Author

@tpburch tpburch commented May 23, 2019

@geek geek added the bug label Jun 14, 2019
@geek geek added this to the 19.0.2 milestone Jun 14, 2019
@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Jun 14, 2019

Fixed by #923

@geek geek closed this Jun 14, 2019
@geek geek self-assigned this Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.