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

Bug #16 - Add selenium, r=johanlorenzo #17

Merged
merged 1 commit into from
Feb 5, 2016

Conversation

michielbdejong
Copy link
Contributor

Work in progress.

@michielbdejong
Copy link
Contributor Author

r=? @JohanLorenzo

I can do a follow-up to add mocha instead of calling selenium-webdriver straight from the main thread of the .js file.

@michielbdejong michielbdejong changed the title [WiP] Add selenium, fix #16 Bug #16 - Add selenium, r=johanlorenzo Feb 4, 2016
script:
- cargo build
- RUST_BACKTRACE=1 cargo test
- ((cargo run ; true) > /dev/null & node test/selenium/ftu_test.js && killall -HUP foxbox) || (killall -HUP foxbox ; exit 1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bash is not that strong, I imagine this line can be improved to look nicer. I did test that it fails/passes on Travis if the test fails/passes, and that in both cases the cargo server is killed, and its exit code is ignored.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could simplify that with:

  - cargo run &> /dev/null & # make an instance run while we run integration tests
  - node test/selenium/ftu_test.js

Would this keep the Travis machine up if we don't kill the foxbox process?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, tried this, but it has to be in the same line.

@michielbdejong michielbdejong force-pushed the add-selenium branch 2 times, most recently from 12ad1e5 to 75522fe Compare February 5, 2016 11:27
@michielbdejong
Copy link
Contributor Author

I removed the step of installing nvm, because it turns out nvm is already installed. I also simplified the test line because I found out stopping the cargo server after the test finished is not necessary.

Splitting commands to separate script lines in .travis.yml doesn't work, the servers are killed in between.

Will make switching to selenium-standalone a follow-up.

script:
- cargo build
- RUST_BACKTRACE=1 cargo test
- cargo run > /dev/null & node test/selenium/ftu_test.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Could you add a ; just between & and node? It'll make the fact that 2 commands are on the same line, more obvious.
cargo run > /dev/null & ; node test/selenium/ftu_test.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That syntax errors with unexpected token ;, but changed it to:
(cargo run > /dev/null &) ; node test/selenium/ftu_test.js

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, it was working under my zsh. I should have checked on bash.

@JohanLorenzo
Copy link
Contributor

r+ @michielbdejong

- "wget http://selenium-release.storage.googleapis.com/2.50/selenium-server-standalone-2.50.1.jar"
- "java -jar selenium-server-standalone-2.50.1.jar > /dev/null &"
- sleep 5
- nvm install 4.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Yay! nvm is already present :)

michielbdejong added a commit that referenced this pull request Feb 5, 2016
Bug #16 - Add selenium, r=johanlorenzo
@michielbdejong michielbdejong merged commit 69675e4 into fxbox:master Feb 5, 2016
@michielbdejong michielbdejong deleted the add-selenium branch February 5, 2016 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants