-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Base reporter store ref to console.log #3725
Changes from all commits
5f64bc2
fff01ff
76e220d
3778c5c
36022ed
47e8dc4
fb3e8bf
3bb6d62
5376b80
19cc3f5
97b3a9d
bc82689
62569cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -277,14 +277,14 @@ describe('XUnit reporter', function() { | |
}); | ||
|
||
describe('when output directed to console', function() { | ||
it("should call 'console.log' with line", function() { | ||
it("should call 'Base.consoleLog' with line", function() { | ||
// :TODO: XUnit needs a trivially testable means to force console.log() | ||
var realProcess = process; | ||
process = false; // eslint-disable-line no-native-reassign, no-global-assign | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mentioned elsewhere, but this test should fail after There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In this PR? Sorry man dont quite follow this. A change relating to this logging? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @plroebuck any chance you could help me with this please. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you not experiencing failure with this specific test? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see.
Passes for me, same if isolate this test. I'll have a look if can find why. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is your concern running function on a falsy value? Seems as its assigned back before the test finishes its there for end of root suite execution. |
||
|
||
var xunit = new XUnit(runner); | ||
var fakeThis = {fileStream: false}; | ||
var consoleLogStub = sinon.stub(console, 'log'); | ||
var consoleLogStub = sinon.stub(Base, 'consoleLog'); | ||
xunit.write.call(fakeThis, expectedLine); | ||
consoleLogStub.restore(); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the "more correct" thing to do is to modify the eslint configuration to handle
console.*
the same way it handles timers (unless that's not desirable, for some reason?)don't think we need to block on that, though; we can change it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a quick look at this. The main issue being that outside the reporters we use
console
(log and error) a fair amount. The solution was always focused on fixing the issue with reporters. We could have a rule just for reporters (seems brittle), or we can overhaul this solution to follow the timers practice, but AFAIK there has been no indication we actually need this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the rules that exist for timers are also specific to certain files, so there’s a precedent for this anyway. again, not a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok i'll have a look at that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ive added the lint rule now.