Skip to content

.only no longer runs all tests having the same name#300

Closed
jtlapp wants to merge 1 commit into
tape-testing:masterfrom
jtlapp:dup-only-fix
Closed

.only no longer runs all tests having the same name#300
jtlapp wants to merge 1 commit into
tape-testing:masterfrom
jtlapp:dup-only-fix

Conversation

@jtlapp

@jtlapp jtlapp commented Jul 17, 2016

Copy link
Copy Markdown
Contributor

This is a fix for #299, wherein all tests having the same name as the test marked .only are run.

@ljharb

ljharb commented Jul 18, 2016

Copy link
Copy Markdown
Collaborator

I wonder if it might be a better idea to just store a reference to the actual test function that's marked "only", rather than adding unique IDs?

@jtlapp

jtlapp commented Jul 18, 2016

Copy link
Copy Markdown
Contributor Author

That would work too, but I would then undo it and redo it with numbers for the feature I've just written. I now have a command line argument -n for showing test numbers and -nX for running only test number X. It works when manually tested, but I'm struggling to write the test suite.

@ljharb

ljharb commented Jul 18, 2016

Copy link
Copy Markdown
Collaborator

I'm skeptical about the utility of that - that immediately makes test ordering tightly coupled to the user's expectations, and adding a test in the middle of the suite would renumber everything.

The numbers appear for TAP output purposes, but I don't think it's a good idea to rely on them beyond referencing a specific test in a specific test run.

@jtlapp

jtlapp commented Jul 18, 2016

Copy link
Copy Markdown
Contributor Author

Everything you say is true, but I don't understand the use case where these problems arise. My use case is that I've run the test suite and have one test I want to work on. My options are:

(1) Insert .only and hope I remember to remove it before checking the source back in or rerunning the test suite with expectations of testing everything. Any time I edit source, I also risk accidentally inserting or deleting characters, just because the mouse is what it is.
(2) Use the extremely ephemeral -n to work this one test case right in the moment without taking risks with source code. Once I've got that one working, I don't care about the number ever again.

@jtlapp

jtlapp commented Jul 18, 2016

Copy link
Copy Markdown
Contributor Author

It's also an option. Leave -n off and no tests get numbered, and people who prefer to modify old test code to get it working can still do so. The option makes people like me more comfortable using the test framework. I really am loath to be changing code that shouldnt' change.

@jtlapp

jtlapp commented Jul 18, 2016

Copy link
Copy Markdown
Contributor Author

I was hoping to have this discussion under its own issue, so I created one #301. I only mentioned the feature here to explain my approach to the present problem.

@ljharb

ljharb commented Jul 18, 2016

Copy link
Copy Markdown
Collaborator

Let's definitely keep discussing in #301.

As for this one, I think this is a good thing to fix - but I'd prefer not to implement the machinery for #301 until that's the direction decided upon.

Would you be willing to rework this to store the function reference, using object identity to determine which test is the "only" one?

@jtlapp

jtlapp commented Jul 18, 2016

Copy link
Copy Markdown
Contributor Author

I agree that if we're not going to be identifying tests by number, then the .only feature can be reference-based.

If we went ahead with a reference-based solution now and later decided to implement -n, then the code would either implement two different ways to select the test to run -- reference and number -- or we rip out the reference solution and restore the number solution.

Given that I only found this problem in pursuit of the -n implementation, I think it's safe to say that there's no urgency to fixing it. People use unique test names.

So I'm thinking that the resolution to this problem can wait on a decision for #301. I just don't see that we should be spending time doing something that we might undo.

@jtlapp

jtlapp commented Jul 18, 2016

Copy link
Copy Markdown
Contributor Author

Besides, people are going to know it the moment that they put .only on a test that is not uniquely named, because they'll get two test results. They would then change one of the names and be on their way. So this bug isn't much of a problem.

@jtlapp

jtlapp commented Aug 2, 2016

Copy link
Copy Markdown
Contributor Author

I'm curious why this pull request is still open if you've already merged the code.

@jtlapp

jtlapp commented Aug 2, 2016

Copy link
Copy Markdown
Contributor Author

Oh, this was my original number-based proposal. Closing.

@jtlapp jtlapp closed this Aug 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants