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

Improve #testing section #169

Merged
merged 32 commits into from
Jun 27, 2016
Merged

Improve #testing section #169

merged 32 commits into from
Jun 27, 2016

Conversation

fniephaus
Copy link
Member

Hi @dalehenrich,

Would you be so kind and skim over this PR?
I have moved the #projects logic from SmalltalkCIGemStone to SmalltalkCI and I have unified the way #classes, #packages and #categories work across all dialects.

This fixes #58 and #135.

Best,
Fabio

@coveralls
Copy link

coveralls commented Jun 24, 2016

Coverage Status

Coverage increased (+2.8%) to 83.002% when pulling fdc96a5 on dev into 43bb330 on master.

@coveralls
Copy link

coveralls commented Jun 24, 2016

Coverage Status

Coverage increased (+2.8%) to 83.002% when pulling fdc96a5 on dev into 43bb330 on master.

@fniephaus
Copy link
Member Author

@dalehenrich I've made a lot of progress on Windows support for Squeak/Pharo that I've already merged into dev. This shouldn't affect GemStone at all. Are you okay with the changes I made to the #testing section?

pharo_zeroconf="$(download_file "get.pharo.org/vm30")" || print_error_and_exit "Pharo-3.0 vm download failed."
bash -c "${pharo_zeroconf}" || print_error_and_exit "Pharo-3.0 vm download failed."
download_file "get.pharo.org/vm30" "$(pwd)/zeroconfig"
bash "$(pwd)/zeroconfig"
popd > /dev/null
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was out of town over the weekend, but now I'm back ... I am concerned that an error during download_file or running the zeroconfig script will not cause the run.sh script to fail and that will mean that it will be difficult to understand what actually failed ...

Copy link
Member Author

Choose a reason for hiding this comment

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

The download_file function now does error handling and exits on error.
See:

curl -f -s -L --retry 3 -o "${target}" "${url}" || print_error_and_exit \

Copy link
Collaborator

Choose a reason for hiding this comment

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

how about the zeroconfig script? That guy still needs to have explicit error test since it is not a script controlled by smalltalkCI

@dalehenrich
Copy link
Collaborator

There are way too many files to look at at this point ... but I think I scanned the important changes and of course the tests are all passing so I think that this looks good ... modulo the zeroconfig script error handling

@fniephaus
Copy link
Member Author

Sorry about that, but thanks for checking the important parts. As I said, download_file exits on error which simplifies the scripts. I'll merge this when I have the time...

pharo_zeroconf="$(download_file "get.pharo.org/30")" || print_error_and_exit "Pharo-3.0 image download failed."
bash -c "${pharo_zeroconf}" || print_error_and_exit "Pharo-3.0 image download failed."
download_file "get.pharo.org/30" "$(pwd)/pharo30_zeroconfig"
bash "$(pwd)/pharo30_zeroconfig"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@fniephaus my concern is that bash "$(pwd)/pharo30_zeroconfig" is no longer protected by a status check ... I understand that download_file does it's own error checking ... but bash "$(pwd)/pharo30_zeroconfig" should be changed to:

bash "$(pwd)/pharo30_zeroconfig" || print_error_and_exit "Pharo-3.0 image download failed."

or something similar ....

Copy link
Collaborator

Choose a reason for hiding this comment

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

...unless bash magically causes run.sh to exit with an error status if the script it is running has a non-zero exit code ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Done: 826c03a

@fniephaus fniephaus merged commit 5073e62 into master Jun 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

What is the purpose of #include section in #testing?
4 participants