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

Defend against console mocks in TAP reporter #1340

Closed
Krinkle opened this issue Dec 18, 2018 · 5 comments · Fixed by #1577
Closed

Defend against console mocks in TAP reporter #1340

Krinkle opened this issue Dec 18, 2018 · 5 comments · Fixed by #1577

Comments

@Krinkle
Copy link
Member

Krinkle commented Dec 18, 2018

Test case

QUnit.module( "e2e/cli", function() {
	QUnit.test( "help", function( assert ) {
		var cli = function () {};
		return cli( "help" ).then( ( data ) => {
			assert.strictEqual( data, true, 'data' );
		} );
	} );
} );

What actually happened?

qunit  test/
TAP version 13
ok 1 Writer > constructor()
ok 2 Writer > child()
ok 3 Writer > prefix()
ok 4 e2e/cli > record
ok 5 e2e/cli > compare
ok 7 e2e/cli > unknown command
ok 8 e2e/conductor > record() - clean state per run
ok 9 conductor > expandScenario()
ok 10 util/is > like() - single type
ok 11 util/is > like() - multi type
ok 12 util/is > like() - plain object
1..12
# pass 11
# skip 0
# todo 0
# fail 1

Notes

Note how there is nothing in the output other than fail 1. After I debugged the whole thing and realised that cli() didn't return a promise (Cannot read property 'then' of undefined"), I did notice there is a tiny little clue.

The clue is, number 6 is skipped in the output (it goes from 5 to 7). So in retrospect, I could've used that to find which test the problem is with, and maybe from there find out what the problem is.

Initially, though, I didn't even know where it was coming from.

@Krinkle Krinkle changed the title Improve general error handling in TAP reporter Defend against console mocks in TAP reporter Dec 24, 2018
@Krinkle
Copy link
Member Author

Krinkle commented Dec 24, 2018

In trying to make a test case, I found that error handling generally does actually work fine.

The problem was that in my specific test case, there was an active mock for the global console object. Specifically, it was a mock applied/removed from hooks.before and hooks.after.

Mocking the object from beforeEach/afterEach works fine.

Krinkle added a commit that referenced this issue Dec 24, 2018
The asserted output is not what we'd like it to be.
This is just to documnet what happens today.

Ref #1340.
Krinkle added a commit that referenced this issue Dec 24, 2018
The asserted output is not what we'd like it to be.
This is just to document what happens today.

Ref #1340.
Krinkle added a commit that referenced this issue Dec 24, 2018
The asserted output is not what we'd like it to be.
This is just to document what happens today.

Ref #1340.
@trentmwillis
Copy link
Member

Perhaps we should save off references to the original functions in the logger?

@Krinkle
Copy link
Member Author

Krinkle commented Dec 24, 2018

@trentmwillis Yeah, something like that.

I haven't tried fixing it yet, but I assumed we'd have to do the ref-saving in js-reporters/TapReporter, rather than in qunit. But.. maybe we need both?

@trentmwillis
Copy link
Member

Ah yes, that is correct. We should do it in both locations.

@Krinkle
Copy link
Member Author

Krinkle commented Nov 7, 2020

Continuing at js-reporters/js-reporters#125.

@Krinkle Krinkle closed this as completed Nov 7, 2020
Krinkle added a commit to js-reporters/js-reporters that referenced this issue Jan 2, 2021
Krinkle added a commit to js-reporters/js-reporters that referenced this issue Jan 2, 2021
Krinkle added a commit to Krinkle/qunit that referenced this issue Apr 4, 2021
Krinkle added a commit that referenced this issue Apr 9, 2021
Krinkle added a commit that referenced this issue Apr 9, 2021
Krinkle added a commit to Krinkle/qunit that referenced this issue Apr 9, 2021
Krinkle added a commit that referenced this issue Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants