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

Test "make release" #1494

Merged
merged 2 commits into from
Apr 9, 2017
Merged

Test "make release" #1494

merged 2 commits into from
Apr 9, 2017

Conversation

PeterDaveHello
Copy link
Collaborator

@PeterDaveHello PeterDaveHello commented Apr 9, 2017 •
edited
Loading

Can help detect one more problem in #1492 and make sure Makefile changes won't break release process!

ljharb
ljharb reviewed Apr 9, 2017
View reviewed changes
Makefile Outdated
@@ -55,14 +55,22 @@ ifndef TAG
$(error Please invoke with `make TAG=<new-version> release`, where <new-version> is either an increment specifier (patch, minor, major, prepatch, preminor, premajor, prerelease), or an explicit major.minor.patch version number)
endif

# Ensures there are version tags in repository
.PHONE: _ensure-current-version
Copy link
Member

Choose a reason for hiding this comment

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

isn't .PHONY more typical?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

typo ... lol

@ljharb
Copy link
Member

ljharb commented Apr 9, 2017

I'm confused; does this just test that make release completes successfully, but doesn't test any of its behavior?

@ljharb ljharb added the testing Stuff related to testing nvm itself. label Apr 9, 2017
@PeterDaveHello
Copy link
Collaborator Author

PeterDaveHello commented Apr 9, 2017 •
edited
Loading

Currently yes, at least it'll catch #1492, I'm not sure what kind of behavior here you expect here?

@ljharb
Copy link
Member

ljharb commented Apr 9, 2017

I guess ensuring that the old version number is completely replaced would be nice :-)

@PeterDaveHello
Copy link
Collaborator Author

PeterDaveHello commented Apr 9, 2017 •
edited
Loading

How would you like to do it? Simply grep certain files under the root of nvm?

@ljharb
Copy link
Member

ljharb commented Apr 9, 2017

:-/ I'm not really sure what the best approach is.

@PeterDaveHello
Copy link
Collaborator Author

PeterDaveHello commented Apr 9, 2017 •
edited
Loading

I don't think we need to reach a perfect or very very good status each time ... actually it's good enough to catch some potential problems already, like #1492, and the other tests are not perfect either, we can just keep making it better.

ljharb
ljharb reviewed Apr 9, 2017
View reviewed changes
Makefile Outdated
@@ -56,7 +56,7 @@ ifndef TAG
endif

# Ensures there are version tags in repository
.PHONE: _ensure-current-version
.PHONY: _ensure-current-version
Copy link
Member

Choose a reason for hiding this comment

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

i think this change belongs to the other commit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Too tired, thanks for catching!

ljharb
ljharb reviewed Apr 9, 2017
View reviewed changes
.travis.yml Outdated
@@ -38,6 +39,7 @@ env:
- PATH="~/.cabal/bin/:$(echo $PATH | sed 's/::/:/')"
- NVM_DIR="${TRAVIS_BUILD_DIR}"
matrix:
- MAKE_RELEASE=true GIT_EDITOR="sed -i '1 s/^/99.99.99 make release test/'"
Copy link
Member

Choose a reason for hiding this comment

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

could we move the GIT_EDITOR var to the script command instead of in the matrix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@ljharb ljharb merged commit 229c7e6 into nvm-sh:master Apr 9, 2017
@PeterDaveHello PeterDaveHello deleted the test-make-release branch April 9, 2017 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
testing Stuff related to testing nvm itself.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@PeterDaveHello @ljharb