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

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

Closed
wants to merge 1 commit into from

Conversation

jtlapp
Copy link
Contributor

@jtlapp jtlapp commented Jul 17, 2016

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

@ljharb
Copy link
Collaborator

ljharb commented Jul 18, 2016

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
Copy link
Contributor Author

jtlapp commented Jul 18, 2016

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
Copy link
Collaborator

ljharb commented Jul 18, 2016

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
Copy link
Contributor Author

jtlapp commented Jul 18, 2016

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
Copy link
Contributor Author

jtlapp commented Jul 18, 2016

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
Copy link
Contributor Author

jtlapp commented Jul 18, 2016

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
Copy link
Collaborator

ljharb commented Jul 18, 2016

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
Copy link
Contributor Author

jtlapp commented Jul 18, 2016

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
Copy link
Contributor Author

jtlapp commented Jul 18, 2016

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
Copy link
Contributor Author

jtlapp commented Aug 2, 2016

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

@jtlapp
Copy link
Contributor Author

jtlapp commented Aug 2, 2016

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