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

fix(karma): ensure job number reflects CI environment #112

Merged
merged 3 commits into from Nov 8, 2017

Conversation

keithamus
Copy link
Member

Fixes #111

@keithamus keithamus requested a review from a team as a code owner November 4, 2017 23:12
@codecov
Copy link

codecov bot commented Nov 4, 2017

Codecov Report

Merging #112 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #112   +/-   ##
=======================================
  Coverage   86.59%   86.59%           
=======================================
  Files           1        1           
  Lines          97       97           
=======================================
  Hits           84       84           
  Misses         13       13

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49b7062...fe0a49e. Read the comment docs.

@keithamus
Copy link
Member Author

Sorry, being lazy and using the web editor. We should squash this down to one commit when merged.

@meeber
Copy link
Contributor

meeber commented Nov 5, 2017

@keithamus With my extremely limited Sauce/Travis/Karma knowledge, I'm not seeing anything obvious regarding the new push and pr failures.

@keithamus
Copy link
Member Author

Looks as though sauce connect is hanging. This is a new one on me 😆

@keithamus
Copy link
Member Author

Oookay it looks like Travis+SauceLabs actually has support for running in PRs (https://docs.travis-ci.com/user/jwt) and so I'm going to take a stab at doing that later this evening, which will hopefully fix this failure as a side effect, and we'll have saucelabs running in PRs which would be awesome.

@keithamus keithamus force-pushed the fix-karma-job-number branch 2 times, most recently from bf19ff5 to b00d2cf Compare November 7, 2017 22:34
@keithamus
Copy link
Member Author

@meeber @vieiralucas I believe this is now green accross the board 😄 🎉. Check out the Travis build logs to see SauceLabs is running in both the push CI and PR CI - meaning that we can now test PRs in IE10, Safari 10, and Edge 13 with Saucelabs, Chrome latest, Firefox latest with built in Travis, and IE11 with Appveyor 🎉

@meeber
Copy link
Contributor

meeber commented Nov 7, 2017

@keithamus Awesome, this looks much cleaner than before. One thing though: although the travis builds are passing, the build logs for the sauce tests make it look like nothing is being tested in IE10, only Safari and Edge. This may just be a logging issue; when I follow the link to the sauce website for the IE 10 tests, the metadata indicates the tests were run:

"totalTime":281,
"skipped":35,
"disconnected":false,
"success":111,
"failed":0,
"netTime":17,
"error":false,
"total":146

Copy link
Member

@vieiralucas vieiralucas left a comment

Choose a reason for hiding this comment

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

@keithamus this is awesome, I'm glad you exists ❤️
Looks Enough To Me (LETM 😄), maybe we can merge this and try to figure out later why travis logs make it look like nothing is being tested.
Unless you are willing to keep digging into it, I'm fine with this being merged as it is.

@keithamus
Copy link
Member Author

Yeah this looks like a logging issue. Let's merge this one, and I can figure out IE10 logging later.

Copy link
Contributor

@meeber meeber left a comment

Choose a reason for hiding this comment

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

LGTM

@meeber meeber merged commit faad27b into master Nov 8, 2017
@meeber meeber deleted the fix-karma-job-number branch November 8, 2017 20:43
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.

None yet

3 participants