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

Upload test results on appveyor builds #382

Merged
merged 1 commit into from
Jun 7, 2016
Merged

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Jun 5, 2016

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Jun 5, 2016
@maxpoulin64
Copy link
Member

That looks neat, 👍

@astorije
Copy link
Member

astorije commented Jun 5, 2016

It does look nice on AppVeyor, but for Travis CI, which I value more than AppVeyor, it looks to me that tests are not even run: https://travis-ci.org/thelounge/lounge/jobs/135370839#L706-L714.
They might be, but since there is no reporting at all, it looks like they aren't (and I'm guessing it's similar in dev env).

Also, new dev dependencies on developers' env while this is really useful only in AppVeyor, but that's the way it is with npm.

We could split paths between Travis CI and AppVeyor so that the reporters differ based on the CI but I think that would overcomplicate things for nothing, and therefore I'd be leaning towards another solution: be happy with the way it is right now. After all, I think it's not that bad. But if you have better to offer, I'm not close to ideas, as long as it doesn't mess up Travis CI and local npm test.

@xPaw xPaw force-pushed the xpaw/upload-appveyor-tests branch from f971ef0 to e8dd83e Compare June 6, 2016 08:38
@xPaw
Copy link
Member Author

xPaw commented Jun 6, 2016

@astorije Good catch, I fixed it by moving --reporter into appveyor config

@astorije astorije force-pushed the xpaw/upload-appveyor-tests branch from 236c478 to 0b6ea60 Compare June 7, 2016 04:36
@astorije
Copy link
Member

astorije commented Jun 7, 2016

@xPaw, this won't work anymore because the npm run test:mocha line has disappeared from the AppVeyor conf in #375.

I am giving it a try by specifying the reporter in a mocha.opts file that is being created during AppVeyor build. That way, all our calls to mocha there are using the reporter regardless of how we write the npm script.
Also, I am installing the reporter during that same build instead of in the package.json file as it's not something that will be used outside of the AppVeyor environment :-)

Note that I created a revert of your initial commit as a second commit before my attempt. Should you not like my solution, you can delete my 2 commits. Should you like it, you can delete yours and the revert :-)
Of course, this is assuming the builds pass... Looks like it went fine after a couple failed attempts, see console and tests tabs. 🎉

@astorije astorije force-pushed the xpaw/upload-appveyor-tests branch from 0b6ea60 to e2f04fe Compare June 7, 2016 04:49
@astorije astorije added the Meta: Do Not Merge This PR should not be merged. label Jun 7, 2016
@astorije
Copy link
Member

astorije commented Jun 7, 2016

(Adding the do not merge label until the revert commit gets removed and the solution decided on.)

@xPaw xPaw force-pushed the xpaw/upload-appveyor-tests branch from e2f04fe to 8addd5a Compare June 7, 2016 14:41
@xPaw xPaw removed the Meta: Do Not Merge This PR should not be merged. label Jun 7, 2016
@xPaw
Copy link
Member Author

xPaw commented Jun 7, 2016

@astorije Brilliant solution! I've rebased the PR.

@@ -13,6 +13,8 @@ environment:
install:
- ps: Install-Product node $env:nodejs_version
- npm install
- npm install mocha-appveyor-reporter
Copy link
Member

Choose a reason for hiding this comment

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

FYI, I had put npm install mocha-appveyor-reporter before npm install on purpose. As it is right now, the reporter install happens after the npm run build task and get lost/clogged up with the rest. Nicer to keep the installs together I think (but really no big deal!).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I don't think that matters.

@xPaw xPaw merged commit d367526 into master Jun 7, 2016
@xPaw xPaw deleted the xpaw/upload-appveyor-tests branch June 7, 2016 20:40
@xPaw xPaw added this to the 2.0.0 milestone Jun 7, 2016
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
…tests

Upload test results on appveyor builds
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants