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

Qunit only #875

Closed
wants to merge 2 commits into from
Closed

Qunit only #875

wants to merge 2 commits into from

Conversation

ebenoist
Copy link
Contributor

Included is a first pass that does not include any changes to the reporter/html.js. I'd love some feedback on how the reporter is normally tested and what patterns I can follow (if any).

Fixes #496

@jquerybot
Copy link

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.


If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

@@ -2,3 +2,4 @@ dist
node_modules
build/report
browserstack-run.pid
temp/
Copy link
Member

Choose a reason for hiding this comment

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

@ebenoist, it was discussed before about adding this temp folder on the gitignore. As it is only created during on grunt failures and it is not persistent, it shouldn't be here.

Although, I don't mind with it being listed here. If the team agrees, I would just leave this folder, better than having further PRs with the folder contents.

@ebenoist ebenoist force-pushed the qunit-only branch 2 times, most recently from aaaf3d7 to dc02d0b Compare October 21, 2015 17:10
@ebenoist
Copy link
Contributor Author

Updated with PR comments addressed. Let me know what you folks think.


newTest.queue();

QUnit.only = QUnit.test; // First only wins, ignore subsequent onlys
Copy link
Member

Choose a reason for hiding this comment

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

Code style issue:

  • Comments are always preceded by a blank line
  • Single line comments go over the line they refer to

@leobalter
Copy link
Member

Note that it won't prevent a var only = QUnit.only; or a ES6's: import {test, only} from 'qunitjs';.

Maybe a check on QUnit.config.filter to assign it only if it's not assigned yet. That way an only call won't win against an URL parameter, but that does not sound as a problem.

Everything else looks great.

@ebenoist
Copy link
Contributor Author

@leobalter I'd prefer not to do the ref swapping either. What is the buy in on this? I'm happy to add some state to QUnit.config that I can interrogate similarly.

@ebenoist
Copy link
Contributor Author

@leobalter I thought about that again, is this what you're thinking?

    // First only wins, ignore subsequent onlys
    if ( !QUnit.config.filter ) {
        QUnit.config.queue.length = 0;
        QUnit.config.filter = newTest.module.name + ": " + newTest.testName;
        newTest.queue();
    }

I just updated the PR with that. I like it.

@ebenoist
Copy link
Contributor Author

It has a nice side effect of ensuring that filter is respected first.

@leobalter
Copy link
Member

It looks great!

@ebenoist
Copy link
Contributor Author

Cool!

if ( !QUnit.config.filter ) {
QUnit.config.queue.length = 0;
QUnit.config.filter = newTest.module.name + ": " + newTest.testName;
newTest.queue();
Copy link
Member

Choose a reason for hiding this comment

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

it sounds weird, but this line should be placed after and outside the if block.

If the filter is already set to run this only test (let's say I click on the rerun button on the reporter), I still need to call newTest.queue().

@ebenoist
Copy link
Contributor Author

Updated!

@leobalter
Copy link
Member

Great! I'll merge it after the CI checks.

Would you help on the docs @ http://github.com/jquery/api.qunitjs.com ?

@ebenoist
Copy link
Contributor Author

I'd be happy to doc it as well.

@ebenoist
Copy link
Contributor Author

qunitjs/api.qunitjs.com#114

Does this work?

@ebenoist
Copy link
Contributor Author

Build flake?

@jzaefferer
Copy link
Member

I guess we currently mix lots of internal state with actual configuration. I'd like to have that improve, instead of adding even more state to that config object.

@ebenoist
Copy link
Contributor Author

@jzaefferer makes sense. The above approach avoids either pitfall. The build still seems to be flaking out though :(

@leobalter
Copy link
Member

@ebenoist I'm running the CI build again, the error was due to a browserstack timeout. It's passing on my machine, thou.

@leobalter
Copy link
Member

I guess we currently mix lots of internal state with actual configuration. I'd like to have that improve, instead of adding even more state to that config object.

I mistook config.filter by config.testId.

Regarding that, an internal state might be better instead of exposing it. A variable would do this work.

@ebenoist, I'm sorry for bringing this extra work, would you mind to replace using QUnit.config.filter to use some local variable?

@ebenoist
Copy link
Contributor Author

By local variable do you mean some state on QUnit.config such as QUnit.config.focused? I can then interrogate that variable when deciding whether to add additional tests to the queue?

@leobalter
Copy link
Member

I mean not using QUnit.config at all, but instead using a local variable at src/test.js

if ( !QUnit.config.filter ) {
QUnit.config.queue.length = 0;
QUnit.config.filter = newTest.module.name + ": " + newTest.testName;
}
Copy link
Member

Choose a reason for hiding this comment

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

Using the variable would be like:

var hasOnly = false;

function only( testName, expected, callback, async ) {

    ...

    if ( !hasOnly ) {
        QUnit.config.queue.length = 0;
        hasOnly = true;
    }

    ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, but I'd still need to interrogate that state when adding additional tests. I can look up that variable on QUnit.test and QUnit.skip calls as well. The filter was effectively doing this for me previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

diff --git a/src/test.js b/src/test.js
index 6078d74..2c619dc 100644
--- a/src/test.js
+++ b/src/test.js
@@ -246,7 +246,6 @@ Test.prototype = {
                },

                test.hooks( "beforeEach" ),
-
                function() {
                    test.run();
                },
@@ -545,6 +544,8 @@ function asyncTest( testName, expected, callback ) {

 // Will be exposed as QUnit.test
 function test( testName, expected, callback, async ) {
+   if ( focused )  { return; }
+
    var newTest;

    if ( arguments.length === 2 ) {
@@ -564,6 +565,8 @@ function test( testName, expected, callback, async ) {

 // Will be exposed as QUnit.skip
 function skip( testName ) {
+   if ( focused )  { return; }
+
    var test = new Test({
        testName: testName,
        skip: true
@@ -572,10 +575,17 @@ function skip( testName ) {
    test.queue();
 }

+var focused = false;
+
 // Will be exposed as QUnit.only
 function only( testName, expected, callback, async ) {
    var newTest;

+   if ( focused )  { return; }
+
+   QUnit.config.queue.length = 0;
+   focused = true;
+
    if ( arguments.length === 2 ) {
        callback = expected;
        expected = null;
@@ -588,11 +598,5 @@ function only( testName, expected, callback, async ) {
        callback: callback
    });

-   // First only wins, ignore subsequent onlys
-   if ( !QUnit.config.filter ) {
-       QUnit.config.queue.length = 0;
-       QUnit.config.filter = newTest.module.name + ": " + newTest.testName;
-   }
-
    newTest.queue();
 }

Something like that?

Copy link
Member

Choose a reason for hiding this comment

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

works for me, but the var focused = false; should go on the top.

@ebenoist
Copy link
Contributor Author

Updated with the diff from the PR and I hoisted the focused var.

@leobalter
Copy link
Member

merged at efe0303. Thanks, @ebenoist!

@ebenoist
Copy link
Contributor Author

Hooray!

leobalter pushed a commit to qunitjs/api.qunitjs.com that referenced this pull request Oct 27, 2015
leobalter pushed a commit to qunitjs/api.qunitjs.com that referenced this pull request Oct 27, 2015
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.

4 participants