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

Do not use hardcoded directory for tests #788

Closed
wants to merge 1 commit into from

Conversation

blino
Copy link

@blino blino commented Oct 15, 2015

The hardcoded /tmp/js-beautify-mkdir directory path is used for tests,
and this directory it not removed after tests.

This can cause multiplie issues on a build server:

  • if multiple builds are running js-beautify tests concurrently, they may try
    to manipulate the directory at the same time, and step on each other's toes
  • if multiple users are using the same build machine, since
    /tmp/js-beautify-mkdir is not removed after tests, the permissions on
    /tmp/js-beautify-mkdir will prevent another user to run tests

The hardcoded /tmp/js-beautify-mkdir directory path is used for tests,
and this directory it not removed after tests.

This can cause multiplie issues on a build server:
- if multiple builds are running js-beautify tests concurrently, they may try
  to manipulate the directory at the same time, and step on each other's toes
- if multiple users are using the same build machine, since
  /tmp/js-beautify-mkdir is not removed after tests, the permissions on
  /tmp/js-beautify-mkdir will prevent another user to run tests
touch -A -01 $JS_BEAUTIFY_MKDIR/js-beautify-old.js
$CLI_SCRIPT -r $JS_BEAUTIFY_MKDIR/js-beautify.js && test $JS_BEAUTIFY_MKDIR/js-beautify.js -nt $JS_BEAUTIFY_MKDIR/js-beautify-old.js && {
echo "js-beautify should not replace unchanged file $JS_BEAUTIFY_MKDIR/js-beautify.js when using -r"
rm -rf $JS_BEAUTIFY_MKDIR
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding this multiple times, could you move this and the exit 1 into a function?

@bitwiseman
Copy link
Member

Great contribution. Could you make the requested tweak, and repeat this change for the other shell testing wrapper scripts?
Thanks!

@bitwiseman bitwiseman added this to the v1.6.0 milestone Dec 8, 2015
@bitwiseman
Copy link
Member

@blino - Ping?

@blino
Copy link
Author

blino commented Jan 21, 2016

Hi,
Sorry, I did not find time to respond with a better fix.
Thanks for finalizing this, and also for the "touch -A" fix!

@bitwiseman
Copy link
Member

You had it mostly done.
No problem. 😄

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.

None yet

2 participants