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

Issue 433: Update sharness version to upstream 1.0.0 version #1035

Merged
merged 7 commits into from Apr 12, 2017

Conversation

Projects
None yet
4 participants
@chu11
Copy link
Contributor

chu11 commented Apr 10, 2017

No description provided.

@chu11 chu11 requested a review from grondo Apr 10, 2017

@chu11 chu11 force-pushed the chu11:issue433 branch from 22e706c to 2b655cf Apr 10, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 10, 2017

Coverage Status

Coverage decreased (-0.02%) to 78.196% when pulling 2b655cf on chu11:issue433 into 6fbe380 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 11, 2017

Codecov Report

Merging #1035 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #1035      +/-   ##
=========================================
+ Coverage   77.89%   77.9%   +0.01%     
=========================================
  Files         150     150              
  Lines       25648   25648              
=========================================
+ Hits        19979   19982       +3     
+ Misses       5669    5666       -3
Impacted Files Coverage Δ
src/common/libflux/response.c 83.76% <0%> (-0.86%) ⬇️
src/common/libflux/handle.c 85.6% <0%> (-0.26%) ⬇️
src/modules/kvs/kvs.c 80.88% <0%> (+0.12%) ⬆️
src/common/libflux/message.c 81.04% <0%> (+0.12%) ⬆️
src/common/libflux/rpc.c 92.3% <0%> (+1.09%) ⬆️

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 632c92f...0e33dec. Read the comment docs.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 11, 2017

Coverage Status

Coverage decreased (-0.03%) to 78.184% when pulling 2b655cf on chu11:issue433 into 6fbe380 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 11, 2017

hmmm, a few builds failed

ERROR: t0000-sharness.t - missing test plan
ERROR: t0000-sharness.t - exited with status 2

Error is from tap. Guess I'm not quite setting something up correctly, or missing some earlier modification.

@chu11 chu11 force-pushed the chu11:issue433 branch from 2b655cf to ae7de6c Apr 11, 2017

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Apr 11, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 11, 2017

Coverage Status

Coverage decreased (-0.03%) to 78.184% when pulling ae7de6c on chu11:issue433 into 6fbe380 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 11, 2017

Looks like I just missed this one for t0000-sharness.t, looks all good now:

< . ./sharness.sh
---
> . `dirname $0`/sharness.sh
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Apr 11, 2017

One change is that trash-directory* is now trash directory* (I.e. Whitespace in directory name). Might want to grep through code, scripts and makefiles auditing for globs or other uses that might break with whitespace. (or just cave and put the dash back in)

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 11, 2017

@grondo I think I got the trash dir one in 226f077 Are you seeing it in something that I'm not?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Apr 11, 2017

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Apr 11, 2017

I hate to suggest it but it would be nice if the forward ported patch stack stayed in separate commits if it is not too much trouble. I guess the squashed commit here at least references the original commits, but it would be easier to submit patches upstream and better for history (IMO) if the changes were kept separate.(reason for changes in the commit body is then kept along with each separate change)

It is a minor thing, so if it is bothersome or difficult let's skip it this time.

On the other hand it would be nice if we weren't modifying upstream sharness at all, so maybe we should try dropping the patches that would never be accepted (passing tests in green, no whitespace in trash directory) and look at submitting the other patches?

@chu11 chu11 force-pushed the chu11:issue433 branch from ae7de6c to 671275c Apr 11, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 11, 2017

@grondo No worries, now that you mention it I should have done the separate commits from the start.

I did bring up an issue with the SHELL_PATH issue (671275c). Obviously, we could just axe a few of the patches as you said. They are just niceties. But I'll admit, I like them (pass is green, no space in trash dir name). I noticed many patches from you upstream, but unsure which patches you made attempts to push upstream and which ones you didn't?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 11, 2017

Coverage Status

Coverage increased (+0.03%) to 78.249% when pulling 671275c on chu11:issue433 into 6fbe380 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Apr 11, 2017

It was very time consuming to push patches upstream, and I didn't submit patches that changed upstream git test-lib.sh behavior (passing tests green, and trash-dir naming are two examples of these). I don't remember why I didn't push some of the other "fixes".

Down the road it might make sense to re-evaluate if we should use git's test-lib.sh instead of sharness and adapt it directly. The git authors took a look at sharness and decided against rebasing git's testsuite on it because sharness was missing a lot of newer functionality of the git testsuite.

If we brought sharness up-to-date with git's more recent test-lib.sh that would be a huge service to the community (but also a huge undertaking, so I'm not sure if it is worth it)

@chu11 chu11 force-pushed the chu11:issue433 branch from 671275c to 36d625b Apr 12, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 12, 2017

Coverage Status

Coverage decreased (-0.03%) to 78.184% when pulling 36d625b on chu11:issue433 into f915009 on flux-framework:master.

chu11 added some commits Apr 7, 2017

testsuite: Update sharness
Update sharness.sh and t0000-sharness.t to upstream 1.0.0 version.

Add test-terminal.perl script from sharness tests.

Update from https://github.com/chriscool/sharness

Fixes #433
testsuite: load sharness.sh from same directory as test
Forward port patch 2e703a7 into
new t0000-sharness.t version 1.0.0.
testsuite: print passing tests in green
Forward port f266983 into sharness.sh
version 1.0.0.
testsuite: Fix sharness tests using SHELL_PATH
Newer sharness.sh sets environment variable SHELL_PATH dependent
on default SHELL of the user.  If tests assume a certain shell
(such as bourne shell) but user's default shell is not compatible,
tests can fail.

Solve by setting individual tests to use the shell they require.

See chriscool/sharness#58 for more detailed
issue description.
testsuite: don't use whitespace in trash directory name
Forward port 1fad056 into sharness.sh
version 1.0.0.
testsuite: support logfile in sharness tests
Forward port 90d815f into sharness.sh
version 1.0.0.

@chu11 chu11 force-pushed the chu11:issue433 branch from 36d625b to 0e33dec Apr 12, 2017

@grondo

grondo approved these changes Apr 12, 2017

Copy link
Contributor

grondo left a comment

LGTM, thanks!

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 12, 2017

Coverage Status

Coverage increased (+0.01%) to 78.158% when pulling 0e33dec on chu11:issue433 into 632c92f on flux-framework:master.

@grondo grondo merged commit e4ceed4 into flux-framework:master Apr 12, 2017

4 checks passed

codecov/patch Coverage not affected when comparing 632c92f...0e33dec
Details
codecov/project 77.9% (+0.01%) compared to 632c92f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 78.158%
Details

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.