Test: Source Displayed Even for Passed Test #758

gauravmittal1995 · 2015-03-03T22:30:28Z

Related to : #706

leobalter · 2015-03-03T22:45:42Z

src/test.js

-			if ( source ) {
-				details.source = source;
-			}
+		source = sourceFromStacktrace();


You can pass a parameter to sourceFromStacktrace to set the exact line you need to cap end send. The default (no parameter) works fine when the result is falsy, but it's not the same when it pass.

The example below - of the tests running with this current changes - seems we need to set the parameter to start with that second line (at file:///Users/leobalter/dev/qunit/test/logs.js:120).

Also, it's probably good to break this information to give just a single line location.

" at temp//@1_Users@1_leobalter@1_dev@1_qunit@1_dist@1_qunit.js:9 at file:///Users/leobalter/dev/qunit/test/logs.js:120 at temp//@1_Users@1_leobalter@1_dev@1_qunit@1_dist@1_qunit.js:9 at temp//@1_Users@1_leobalter@1_dev@1_qunit@1_dist@1_qunit.js:9 at process (temp//@1_Users@1_leobalter@1_dev@1_qunit@1_dist@1_qunit.js:9) at begin (temp//@1_Users@1_leobalter@1_dev@1_qunit@1_dist@1_qunit.js:9) at temp//@1_Users@1_leobalter@1_dev@1_qunit@1_dist@1_qunit.js:9"

gauravmittal1995 · 2015-03-03T23:45:37Z

@leobalter Done. Please Review

leobalter · 2015-03-04T01:37:04Z

src/test.js

-			}
+		source = sourceFromStacktrace( 4 );
+		if ( source ) {
+			details.source = source;


As I mentioned before, the sourceFromStacktrace parameter must be different when the test fail, otherwise you're just gonna solve the problem when it pass, before it was good when the tests failed.

jzaefferer · 2015-03-04T09:15:14Z

#706 is about showing the location of passed tests(!), not passed assertions.

gauravmittal1995 · 2015-03-04T12:20:16Z

@jzaefferer @leobalter Made Changes. Please Review

jzaefferer · 2015-03-04T12:29:25Z

The output needs to be inside the collapsed element, so that its only shown when opening details of a test. Currently this source line is shown for every test, which looks pretty bad.

gauravmittal1995 · 2015-03-04T18:18:37Z

@jzaefferer Made it collapsable. Please Review

leobalter · 2015-03-04T18:21:28Z

test/reporter-html.js

@@ -106,7 +106,7 @@ QUnit.test( "setup", function( assert ) {
 QUnit.test( "basics", function( assert ) {
 	assert.expect( 2 );
 	var previous = getPreviousTests( /^setup$/, /^timing$/ )[ 0 ],
-		runtime = previous.lastChild.previousSibling;
+		runtime = previous.lastChild.previousSibling.previousSibling;


Here's a good opportunity to include tests for these changes.

Would you include a test with a regexp containing the static part of the source line?

leobalter · 2015-03-04T18:24:42Z

Working good for me, now it's just about to add a test and fix the code style issues.

leobalter · 2015-03-04T18:25:30Z

reporter/html.js

@@ -723,6 +727,10 @@ QUnit.testDone(function( details ) {
 		time.innerHTML = details.runtime + " ms";
 		testItem.insertBefore( time, assertList );
 	}
+
+	sourceName.innerHTML = "<b class='counts'>Source: </b>" + details.source;
+	assertList.parentNode.insertBefore(sourceName, assertList.nextSibling);


missing space before and after arguments list.

assertList.parentNode.insertBefore( sourceName, assertList.nextSibling );

I was playing with the code and we can do this easier:

sourceName = document.createElement( "p" ); sourceName.innerHTML = "<strong>Source: </strong>" + details.source; addClass( sourceName, "qunit-collapsed qunit-source" ); addEvent( testTitle, "click", function() { toggleClass( sourceName, "qunit-collapsed" ); }); testItem.appendChild( sourceName );

The source can be inserted to any test

it shouldn't use counts class, but just a strong tag on the Source: text.

the source element now uses a qunit-source class.

the click event is made individually, this way you can still have the source information on skipped tests

it is appended on the testItem element.

@leobalter I tried the above. But gives error for addEvent because it calls addevent two time for testTitle click.
Changed innerhtml to strong and used appendChild instead of addBefore.
Seems to be working fine now.

gauravmittal1995 · 2015-03-04T18:34:23Z

@leobalter Hey, What test is needed? Can u explain a bit more?

gauravmittal1995 · 2015-03-04T19:42:10Z

@leobalter Hey, Made the changes. Can u explain the test that is needed??

leobalter · 2015-03-04T19:43:59Z

you need to add test for the changes you made, assert the previous test added a source line.

leobalter · 2015-03-04T19:44:24Z

also: #758 (comment)

gauravmittal1995 · 2015-03-04T19:48:31Z

@leobalter How do i add test for the change??

jzaefferer · 2015-03-04T22:02:02Z

HTML reporter output for passing tests looks good to me now. May want to hide that for failing tests, since the failing assertion already includes source lines.

leobalter · 2015-03-04T22:06:42Z

I'm writing the tests and will land them as soon as I fix this PR on IE6 to 9, they're currently saying "Source: undefined"

gauravmittal1995 · 2015-03-04T22:43:33Z

@jzaefferer Hey, IMHO, i think that we should keep the source even for failed test. Since failed test only show lines of fail assertion, a failed test can still have some passed assertions as well. Also the test can be big and the assertion called much later than where the test starts.
Also this will also be consistent with the passed tests.

This is what i think. I could be wrong. Please let me know if i should hide it for failed assertions.

gauravmittal1995 · 2015-03-06T19:35:57Z

@leobalter Any change needed? or should i squash the commits?

leobalter · 2015-03-06T19:45:32Z

@gauravmittal1995, let the squash to happen only before merging. So far we need to wait @jzaefferer to answer my comments or you can try the approach he suggested, in a new commit in the PR.

If @jzaefferer agrees with my arguments, we're probably good to land, but we need to wait the next time he's available. Different time zones makes it take more time.

gauravmittal1995 · 2015-03-06T19:49:03Z

@leobalter I think i will wait for @jzaefferer to reply to your comments before making the changes he asked for

jzaefferer · 2015-03-09T16:56:00Z

Discussed this with @leobalter in a private chat. He's going to do some tests to verify support for Error().stack, which should help making an informed decision here.

gauravmittal1995 · 2015-03-09T17:40:52Z

@jzaefferer ok ... thanks

Ref qunitjs#758

jzaefferer · 2015-03-10T20:20:45Z

@leobalter since #773 doesn't change much, what's the verdict for this PR?

leobalter · 2015-03-10T20:27:15Z

I don't like to add tests based on the log to verify the feature. But we can replace that if ( Error.stack() ) by something related to details.source if is defined in a testDone after the "setup" test (which would require to hook removal later).

Other way, without adding logging hooks:

In the tests just copy the content from sourceFromStacktrace (without the return statement) for to replace the if condition in the tests.

jzaefferer · 2015-03-10T20:34:15Z

I don't like to add tests based on the log to verify the feature. But we can replace that if ( Error.stack() ) by something related to details.source if is defined in a testDone after the "setup" test (which would require to hook removal later).

I'm not sure what you're suggesting here. Can you please clarify, maybe using an example?

In the tests just copy the content from sourceFromStacktrace (without the return statement) for to replace the if condition in the tests.

That seems fine, since all the complexity lives in the extract... method.

leobalter · 2015-03-10T20:36:26Z

I didn't write an example before because it would be long and ugly, but as you agreed with the second option I would just stick with that.

jzaefferer · 2015-03-10T20:39:43Z

👍

Ref qunitjs#758

leobalter · 2015-03-26T16:37:35Z

@gauravmittal1995, #773 is landed, let's finish this one!

leobalter · 2015-03-27T02:33:08Z

I've got the branch and fixed this, but I got one remaining issue.

Safari 5.1 fails the tests as the stack is not defined because of this line

I can't have the same detection on the test js file. There's nothing beyond the userAgent to avoid this.

leobalter · 2015-03-27T03:28:24Z

https://github.com/jquery/qunit/compare/jquery:master...leobalter:gauravmittal1995-Source?expand=1

Here's the 2 commits fixing this PR. My branch is also rebased with master.

The second one is not the best thing as it determines Safari 5.1 (the sourceURL exception) via userAgent. I can't find anything else to avoid this.

jzaefferer · 2015-03-27T11:24:02Z

jQuery Core exports $.support, even though it mostly uses those flags internally. How about we export QUnit.stacktraceSupportIsThere (not my naming day) and use that in tests? Maybe we can use the flag internally as well, not sure.

leobalter · 2015-03-27T15:06:55Z

QUnit.stack should be fine and would export a standardized Error stack. I'll push this in a separate PR so we can proper discuss it.

For now, we can land this as the flags are already internal (right?), and replace the tests when QUnit.stack is ready, or wait until the other PR to land.

jzaefferer · 2015-03-27T15:25:14Z

Let's create a separate issue and merge this.

From internal sourceFromStacktrace method Ref qunitjs#758

From internal sourceFromStacktrace method Ref qunitjs#758 Closes qunitjs#801

Closes qunitjs#758

gauravmittal1995 · 2015-03-27T22:53:20Z

@leobalter Do i rebase this with https://github.com/jquery/qunit/compare/jquery:master...leobalter:gauravmittal1995-Source?expand=1 ???

From internal sourceFromStacktrace method Ref qunitjs#758 Closes qunitjs#801

Closes qunitjs#758

From internal sourceFromStacktrace method Ref qunitjs#758 Closes qunitjs#801

jquerybot added the CLA: Valid label Mar 3, 2015

leobalter reviewed Mar 3, 2015
View reviewed changes

gauravmittal1995 force-pushed the Source branch from b65cba9 to 792faab Compare March 3, 2015 23:34

leobalter reviewed Mar 4, 2015
View reviewed changes

gauravmittal1995 force-pushed the Source branch from 792faab to 688a6a5 Compare March 4, 2015 12:05

gauravmittal1995 force-pushed the Source branch from 688a6a5 to 31c0c00 Compare March 4, 2015 18:17

leobalter reviewed Mar 4, 2015
View reviewed changes

gauravmittal1995 force-pushed the Source branch from 31c0c00 to 8c692cc Compare March 4, 2015 19:37

Test: Source Displayed Even for Passed Test

0dc12dd

gauravmittal1995 force-pushed the Source branch from 8c692cc to 0dc12dd Compare March 4, 2015 19:40

leobalter added 2 commits March 4, 2015 16:44

Tests: Cleanup HTML reporter timing tests

15ed6de

HTML Reporter: Add tests to the source information

7835c38

HTML Reporter: Source info is only present if Error.stack is supported

d9de576

shivamdixit mentioned this pull request Mar 9, 2015

Show source location of passed tests #706

Closed

leobalter added a commit to leobalter/qunit that referenced this pull request Mar 9, 2015

Core: Simplify stack trace methods

c92ccd6

Ref qunitjs#758

leobalter mentioned this pull request Mar 9, 2015

Core: Simplify stack trace methods #773

Merged

leobalter added a commit to leobalter/qunit that referenced this pull request Mar 26, 2015

Core: Simplify stack trace methods

6e828ed

Ref qunitjs#758

leobalter added a commit to leobalter/qunit that referenced this pull request Mar 27, 2015

Core: Implement QUnit.stack

69763fd

From internal sourceFromStacktrace method Ref qunitjs#758

leobalter mentioned this pull request Mar 27, 2015

QUnit.stack #801

Closed

leobalter added a commit to leobalter/qunit that referenced this pull request Mar 27, 2015

Core: Implement QUnit.stack

026a3e8

From internal sourceFromStacktrace method Ref qunitjs#758 Closes qunitjs#801

leobalter pushed a commit to leobalter/qunit that referenced this pull request Mar 27, 2015

Test: Source Displayed Even for Passed Test

477e1f1

Closes qunitjs#758

leobalter added a commit to leobalter/qunit that referenced this pull request Apr 13, 2015

Core: Implement QUnit.stack

cdf8725

From internal sourceFromStacktrace method Ref qunitjs#758 Closes qunitjs#801

leobalter pushed a commit to leobalter/qunit that referenced this pull request Apr 13, 2015

Test: Source Displayed Even for Passed Test

287a972

Closes qunitjs#758

leobalter added a commit to leobalter/qunit that referenced this pull request Apr 14, 2015

Core: Implement QUnit.stack

045de95

From internal sourceFromStacktrace method Ref qunitjs#758 Closes qunitjs#801

leobalter closed this in 9498355 Apr 14, 2015

Test: Source Displayed Even for Passed Test #758

Test: Source Displayed Even for Passed Test #758

Conversation

gauravmittal1995 commented Mar 3, 2015

leobalter Mar 3, 2015

Choose a reason for hiding this comment

gauravmittal1995 commented Mar 3, 2015

leobalter Mar 4, 2015

Choose a reason for hiding this comment

jzaefferer commented Mar 4, 2015

gauravmittal1995 commented Mar 4, 2015

jzaefferer commented Mar 4, 2015

gauravmittal1995 commented Mar 4, 2015

leobalter Mar 4, 2015

Choose a reason for hiding this comment

leobalter commented Mar 4, 2015

leobalter Mar 4, 2015

Choose a reason for hiding this comment

leobalter Mar 4, 2015

Choose a reason for hiding this comment

gauravmittal1995 Mar 4, 2015

Choose a reason for hiding this comment

gauravmittal1995 commented Mar 4, 2015

gauravmittal1995 commented Mar 4, 2015

leobalter commented Mar 4, 2015

leobalter commented Mar 4, 2015

gauravmittal1995 commented Mar 4, 2015

jzaefferer commented Mar 4, 2015

leobalter commented Mar 4, 2015

gauravmittal1995 commented Mar 4, 2015

gauravmittal1995 commented Mar 6, 2015

leobalter commented Mar 6, 2015

gauravmittal1995 commented Mar 6, 2015

jzaefferer commented Mar 9, 2015

gauravmittal1995 commented Mar 9, 2015

jzaefferer commented Mar 10, 2015

leobalter commented Mar 10, 2015

jzaefferer commented Mar 10, 2015

leobalter commented Mar 10, 2015

jzaefferer commented Mar 10, 2015

leobalter commented Mar 26, 2015

leobalter commented Mar 27, 2015

leobalter commented Mar 27, 2015

jzaefferer commented Mar 27, 2015

leobalter commented Mar 27, 2015

jzaefferer commented Mar 27, 2015

gauravmittal1995 commented Mar 27, 2015