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

reference-based .only fix for #299 #303

Merged
merged 2 commits into from
Aug 2, 2016

Conversation

jtlapp
Copy link
Contributor

@jtlapp jtlapp commented Aug 1, 2016

.only now identifies tests by reference instead of by test name, fixing #299

@@ -22,6 +22,7 @@ function Results () {
this.pass = 0;
this._stream = through();
this.tests = [];
this._only = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can probably be initialized to null, or function () {}, rather than zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, my preference is to initialize to null. I was trying to be consistent with the rest of the code, as you had asked. ;-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The other things that init to 0 tho end up assigned to numbers, right? ie, the convention is to initialize such that the type of the property doesn't change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, just checked through them. Maybe I was confusing those with another project. I'll fix. To minimize the chance of me screwing up, it would help for you to point me to the right way to update a PR. I screwed up a PR on a project once so that thereafter every single PR indicated failed merges despite merges really being clean. I'll spend an hour researching before making an attempt. I've gotten git-shy.

Copy link
Collaborator

Choose a reason for hiding this comment

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

np - basically you just rebase your local branch as you like - without deleting the branch or pushing any changes to the remote - and then, assuming "origin" is the remote that points to your fork, git push origin ref-based-only -u -f - without the "-f" it will refuse to push it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, what did you want to accomplish with the rebase? I'm getting Current branch ref-based-only is up to date. My master branch has edits, so I'm working off of a source branch that's a copy of your master. That's the message I get on git rebase source.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You'd need to do a fetch off the source remote, and then git rebase source/master

@ljharb ljharb added the bug label Aug 1, 2016
@ljharb ljharb self-assigned this Aug 1, 2016
@jtlapp
Copy link
Contributor Author

jtlapp commented Aug 2, 2016

It appears to have updated properly. Now to add notes to my already-huge set so notes on git.

@ljharb ljharb merged commit 87ee0ac into tape-testing:master Aug 2, 2016
@jtlapp
Copy link
Contributor Author

jtlapp commented Aug 2, 2016

I can't git rebase upstream/master from this merge, even after doing a git fetch upstream. I get this, indicating that it doesn't see the changes you just merged in:

--- a/index.js
+++ b/index.js
@@ -138,9 +138,10 @@ function createHarness (conf_) {
     var only = false;
     test.only = function (name) {
         if (only) throw new Error('there can only be one only test');
-        results.only(name);
+        var t = test.apply(null, arguments);
+        results.only(t.number);
         only = true;
-        return test.apply(null, arguments);
+        return t;
     };
     test._exitCode = 0;

Note that I'm not working off of a child branch of the branch of mine that you merged.

@jtlapp
Copy link
Contributor Author

jtlapp commented Aug 3, 2016

I attempted another approach that also didn't work:

  • I created a new branch and pulled from upstream/master.
  • I merged in my hook changes into this branch.
  • I attempted to git rebase upstream/master but once again got unreasonable merge conflicts.

It's telling me that the upstream branch is using the old name-based only determinations, and that my current branch is using number-based determinations. Both are true for past commits in each branch, but neither is true for the current state of things. Am I supposed to resolve prior conflicts that have since been removed?

@jtlapp
Copy link
Contributor Author

jtlapp commented Aug 3, 2016

Okay I figured it out. I had to walk the rebase forward through time, even if it meant temporarily adding back in code that has since been deleted.

ljharb added a commit that referenced this pull request Aug 5, 2016
ljharb added a commit that referenced this pull request Sep 30, 2016
 - [Fix] `throws`: only reassign “message” when it is not already non-enumerable (#320)
 - [Fix] show path for error messages on windows (#316)
 - [Fix] `.only` should not run multiple tests with the same name (#299, #303)
 - [Deps] update `glob`, `inherits`
 - [Dev Deps] update `concat-stream`, `tap`, `tap-parser`, `falafel`
 - [Tests] [Dev Deps] Update to latest version of devDependencies tap (v7) and tap-parser (v2) (#318)
 - [Tests] ensure the max_listeners test has passing output
 - [Docs] improvements (#298, #317)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants