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 branch for coveralls badge #26561

Merged
merged 3 commits into from May 23, 2018
Merged

fix branch for coveralls badge #26561

merged 3 commits into from May 23, 2018

Conversation

Johann-S
Copy link
Member

/CC @XhmikosR

@XhmikosR
Copy link
Member

@Johann-S: can you change it to shields.io?

@coveralls
Copy link

Pull Request Test Coverage Report for Build 27201

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 88.53%

Totals Coverage Status
Change from base Build 27199: 0.0%
Covered Lines: 1331
Relevant Lines: 1467

💛 - Coveralls

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build 27201

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 88.53%

Totals Coverage Status
Change from base Build 27199: 0.0%
Covered Lines: 1331
Relevant Lines: 1467

💛 - Coveralls

@Johann-S
Copy link
Member Author

Wow we should just run coverall for one build not for all 😆 and done @XhmikosR 😉

@XhmikosR
Copy link
Member

I'm gonna change the settings to stop Coveralls from commenting.

@Johann-S
Copy link
Member Author

Not sure if it's the solution, because it's interesting to know if a PR decrease or increase our Coverage 😟

@XhmikosR
Copy link
Member

@Johann-S: how about we move the coveralls command from the npm scripts to travis only?

@XhmikosR
Copy link
Member

The diff shows @Johann-S. We just won't get spammed by it.

@Johann-S
Copy link
Member Author

Yep sure, if it's allow us to run it just once it would be perfect ! 👍

@Johann-S
Copy link
Member Author

Oh yes you're right, I wasn't sure about that 😄

@XhmikosR
Copy link
Member

@Johann-S: not sure why, but every time you change the package-lock.json you make many changes. Please make sure you are using npm that ships with node 8 or 10 and not another version. I'm gonna revert the changes, but we need to make sure it doesn't happen again.

@Johann-S
Copy link
Member Author

I use npm v6.0.0 and node v9.7.1 😟

@XhmikosR
Copy link
Member

Yeah, please move to 8 or 10.

@XhmikosR
Copy link
Member

OK, so this runs coveralls after success, but for both builds. It doesn't seem to affect anything.

@Johann-S
Copy link
Member Author

and no more comments 🎆

@XhmikosR
Copy link
Member

I could move the after_success call to run only after node.js 8. What do you think about that?

@Johann-S
Copy link
Member Author

Much better to just run it once 👍

@Johann-S
Copy link
Member Author

Or you can revert that change and let coveralls command in our package.json as we do not have Coveralls comments

@XhmikosR
Copy link
Member

OK, I think we just need a little shell help. See my last push.

@Johann-S
Copy link
Member Author

I made a change, which run coveralls just for node 8, can you take a look @XhmikosR ?

@XhmikosR
Copy link
Member

Does the job indeed!

But I think we should not export another variable and just do the version check in the if check.

Something like this: https://stackoverflow.com/questions/48250235/how-to-setup-travis-to-skip-script-on-particular-nodejs-version

@Johann-S
Copy link
Member Author

Johann-S commented May 23, 2018

What do you think about that :

- if [[ `node -v` = 8* ]]; then npm run coveralls; fi

Finally I doesn't work... 😢
I'll try another thing

@Johann-S Johann-S force-pushed the v4-dev-jo-fix-readme branch 4 times, most recently from 0ace573 to 93e40e8 Compare May 23, 2018 09:46
@XhmikosR XhmikosR added this to Inbox in v4.1.2 via automation May 23, 2018
@XhmikosR
Copy link
Member

LGTM @Johann-S, feel free to rebase and merge :)

@XhmikosR
Copy link
Member

@Johann-S: I just become aware there's a TRAVIS_NODE_VERSION. Might be worth checking if this variable is something we can just use.

@XhmikosR
Copy link
Member

OK, it works :) I'm gonna merge when the build finishes.

@XhmikosR XhmikosR merged commit c2b13b9 into v4-dev May 23, 2018
v4.1.2 automation moved this from Inbox to Shipped May 23, 2018
@XhmikosR XhmikosR deleted the v4-dev-jo-fix-readme branch May 23, 2018 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v4.1.2
  
Shipped
Development

Successfully merging this pull request may close these issues.

None yet

3 participants