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.

Sign up for GitHub

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

Jump to bottom

Make coveralls watch the include folder #957

Merged
merged 1 commit into from
Feb 12, 2018
Merged

Make coveralls watch the include folder #957

merged 1 commit into from
Feb 12, 2018

Conversation

theodelrieu
Copy link
Contributor

Should be enough to fix it.

@coveralls
Copy link

coveralls commented Feb 4, 2018 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling 41db7cd on theodelrieu:fix_coveralls into 447f542 on nlohmann:develop.

@theodelrieu
Copy link
Contributor Author

The latest Coveralls job passed: https://coveralls.io/jobs/33439123

Could you retry the Travis job that failed? Seems like a spurious failure...

@theodelrieu
Copy link
Contributor Author

theodelrieu commented Feb 4, 2018 •
edited
Loading

Oops, seems my fix didn't fix anything after all...

EDIT: That's a shameful typo, missing S to MultipleHeaders

@theodelrieu theodelrieu force-pushed the fix_coveralls branch 3 times, most recently from ad29f12 to 70c7ba6 Compare February 5, 2018 14:10
@nlohmann
Copy link
Owner

nlohmann commented Feb 6, 2018

coveralls

This is how Coveralls looks like right now. The 0% at the top is strange - and this is also the number reported in this PR. Any idea how this can be fixed?

@theodelrieu
Copy link
Contributor Author

That's very strange, maybe because we give include instead of include/nlohmann, I'll give it a try.

@theodelrieu theodelrieu force-pushed the fix_coveralls branch 2 times, most recently from d0c7396 to 77eafce Compare February 8, 2018 10:29
@theodelrieu
Copy link
Contributor Author

I might become crazy with Coveralls... @nlohmann I remember I could try locally before, but the private repo's token has been moved from the repo files (which is good!)

Could you try to fix this one? Running Travis for hours to finally see a big fat 0.0% is quite frustrating...

@nlohmann
Copy link
Owner

nlohmann commented Feb 9, 2018

I'll have a look.

@nlohmann
Copy link
Owner

nlohmann commented Feb 9, 2018

See comments in #953. With 2dda87c, coverage data should be generated again. I am currently not planning to extend this to the multi-header version myself, but I think this can be done with a PR, because Travis/Coveralls seems not to care about who is comitting.

@theodelrieu
Copy link
Contributor Author

It seems CMake does not go well with environment variables which contains spaces...

Hence the MULTIPLE_HEADERS I've added in the travis file

@theodelrieu
Copy link
Contributor Author

I think it's good now, finally!

nlohmann
nlohmann reviewed Feb 12, 2018
View reviewed changes
.travis.yml
@@ -281,14 +282,16 @@ script:

# make sure CXX is correctly set
- if [[ "${COMPILER}" != "" ]]; then export CXX=${COMPILER}; fi
# by default, use the single-header version
- if [[ "${MULTIPLE_HEADERS}" == "" ]]; then export MULTIPLE_HEADERS=OFF; fi
Copy link
Owner

Choose a reason for hiding this comment

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

I did not think of this trick. Well done!

nlohmann
nlohmann approved these changes Feb 12, 2018
View reviewed changes
Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@nlohmann nlohmann self-assigned this Feb 12, 2018
@nlohmann nlohmann added this to the Release 3.1.1 milestone Feb 12, 2018
@nlohmann nlohmann merged commit b02e3bb into nlohmann:develop Feb 12, 2018
@theodelrieu theodelrieu deleted the fix_coveralls branch March 13, 2019 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nlohmann nlohmann nlohmann approved these changes

Assignees

@nlohmann nlohmann

Labels
None yet
Projects
None yet
Milestone
Release 3.1.1
Development

Successfully merging this pull request may close these issues.
3 participants
@theodelrieu @coveralls @nlohmann