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

feat: add support for jasmine-spec-reporter (displayStacktrace: true) #70

Merged
merged 4 commits into from
Aug 24, 2017

Conversation

qualityshepherd
Copy link
Contributor

Adds support for jasmine-spec-reporter to standard parser. Also, very likely resolves #54

@qualityshepherd
Copy link
Contributor Author

qualityshepherd commented Aug 23, 2017

@NickTomlin there seems to be a bug either in your transpile, or my brain, because my change here is not making it into the dist folder. My money's on my brain 'cause I don't totally get what you're doing here.

It works in isolation (eg. when I run the unit tests), but it's not updating the dist folder, when including in my protractor project.

That said, this simple change should fix #54 that folks (and myself) are having.

@NickTomlin
Copy link
Owner

NickTomlin commented Aug 24, 2017

Awesome!

Could you add a spec for this?

I'll take a transpilation stuff. Update: it's building for me locally. What happens when you run npm run build?

@qualityshepherd
Copy link
Contributor Author

qualityshepherd commented Aug 24, 2017

Ahhh... so this is an issue with jasmine-spec-reporter and Protractor 5.1.2. You're using 4.x, which still prints at Object.<anonymous>. 5.1.2 prints at UserContext.<anonymous>.

You want me to update the protractor version, or just add the output and a unit test (which I'm trying to figure out what the hell you're doing in there :) )?

Update: oh, and the transpile issue was just while I was debugging via node_modules in my code, which was throwing wackiness (stupid transpilers ;) ). It works fine locally.

@NickTomlin
Copy link
Owner

NickTomlin commented Aug 24, 2017

I'd add a unit test but feel free to bump the protractor version while you are at it.

@qualityshepherd
Copy link
Contributor Author

(also asked on Gitter)
I added a flakey test output file for jasmine-spec-reporter, and the following unit test:

    it('isolates individual failed specs from jasmine-spec-reporter output', () => {
      protractorFlake({maxAttempts: 3})

      spawnStub.dataCallback(failedJasmineSpecReporterTestOutput)
      spawnStub.endCallback(1)

      expect(spawnStub).to.have.been.calledWith('node', [pathToProtractor(), '--params.flake.retry', true, '--specs', '/tests/a-flakey.test.js'])
    })

but it's failing with this error:

  1) Protractor Flake failed specs isolates individual failed specs from jasmine-spec-reporter output:
     AssertionError: expected stub to have been called with arguments node, ["/Users/brine/protractor-flake/node_modules/protractor/bin/protractor", "--params.flake.retry", true, "--specs", "/tests/a-flakey.test.js"]%D
      at Context.<anonymous> (test/unit/index.test.js:103:38)

What am I doon wrong?

@qualityshepherd
Copy link
Contributor Author

Got it... was just braindumb. Ready for review, Nick...

Copy link
Owner

@NickTomlin NickTomlin left a comment

Choose a reason for hiding this comment

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

Some very minor changes. Other than that this looks great!

My commit linter is being super unhelpful and breaking the build so I can merge this in myself once you've updated.

@@ -0,0 +1,51 @@
'use strict';
Copy link
Owner

Choose a reason for hiding this comment

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

I'd remove this file unless we need it since (we don't have an integration test that actually uses 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.

Cool... wasn't sure if you wanted a repeatable conf for the future [WOOSH SOUND]. The answer is no :) It's gone.

package.json Outdated
@@ -37,10 +37,11 @@
"cookie-parser": "^1.3.5",
"express": "^4.13.1",
"express-session": "^1.11.3",
"jasmine-spec-reporter": "^4.2.1",
Copy link
Owner

Choose a reason for hiding this comment

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

Is this dependency actually needed? I'm fine with omitting it since we have generated the static test output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not now that the conf using it is gone

@wswebcreation
Copy link
Collaborator

@qualityshepherd

Tnx for this PR, I didn't had the time to look at this because the sun didn't let me (something called vacation 😁).

Maybe something not in scope of this change, but is there a way in Jasmine that we can log a line of text to the console on every failed test?

That would make the reading of the log for protractor-flake much easier. Currently working on that for the CucumberJS parser

@qualityshepherd
Copy link
Contributor Author

@wswebcreation yeah, definitely out of scope for this pr, but would be easy enough to do...

@NickTomlin all wrapped up. Though one last question... do we want this fix in multi too?

@wswebcreation
Copy link
Collaborator

wswebcreation commented Aug 24, 2017

@qualityshepherd

I would advice to also fix the multi

Secondly, if it's easy to fix for Jasmine with a logline, can you give me a direction so I can create aPR for CucumberJS and Jasmine with examples?

@NickTomlin
Copy link
Owner

@wswebcreation @wswebcreation great point on multi, I always forget about that one :( I think we should!

It would be interesting to expose separate reporters for jasmine/cucumber that do this logging. Let's keep that discussion going in another PR. Do one of you want to open up an issue to track it?

@wswebcreation
Copy link
Collaborator

Will create an issue for this

@qualityshepherd
Copy link
Contributor Author

@NickTomlin cool... that was easy, as the multi parser doesn't needs a change for this. So I just added a unit test.

@wswebcreation
Copy link
Collaborator

@NickTomlin and @qualityshepherd

I've created 2 new issues (Feature request), #71 and #72.

@NickTomlin
Copy link
Owner

NickTomlin commented Aug 24, 2017

I've gone ahead and removed conventional changelog linting on master, so could you rebase to re-run the integration tests? In my other runs it seems like bumping protractor breaks the integration tests. We should either fix them or downgrade for now and bump later.

Edit: we can bump the version but we'll need to drop node 4+5 from the builds (and we should probably add 8 while we are there)

@NickTomlin NickTomlin merged commit f9229b5 into NickTomlin:master Aug 24, 2017
NickTomlin added a commit that referenced this pull request Aug 24, 2017
@NickTomlin
Copy link
Owner

I went ahead and made these changes myself and merged this in. Thanks!

@qualityshepherd
Copy link
Contributor Author

Cool... thanks Nick!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Protractor-flake running again the tests that have passed
3 participants