-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Selenium test for PR 26979 #27057
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
Selenium test for PR 26979 #27057
Conversation
the tests are run on http://saucelabs.com/
only run the selenium tests whe SAUCE_USERNAME & SAUCE_ACCESS_KEY are configured. That way a PR from an external, that does not have access to the sauce env., will not trigger the selenium tests
…al-it/owncloud-core into selenium-implementation
login into oC with admin credentials
Everything in this branch looks good.
|
@individual-it, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975 and @PVince81 to be potential reviewers. |
| @@ -0,0 +1,49 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please do not add the vendor directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so do you want the webdriver be in tests/facebook/webdriver ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add this as dev dependency into /composer.json
.travis.yml
Outdated
| before_install: | ||
| - make | ||
| - sh -c "if [ '$TEST_DAV' = '1' ]; then bash tests/travis/before_install.sh $DB; fi" | ||
| - if [ ! -z "$SAUCE_USERNAME" ] && [ ! -z "$SAUCE_ACCESS_KEY" ]; then export TEST_SELENIUM=1 ; else export TEST_SELENIUM=0 ; fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add this as a test case just like we do with caldav, carddav.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is to prevent the tests to run if a PR comes from a remote fork.
So if we make it a test-case by setting TEST_SELENIUM=1 and the sauce_connector is setup in owncloud/core and I make a fork to individual-it/core commit something and make a PR. Travis would try to build that PR but the selenium tests would fail because $SAUCE_USERNAME & $SAUCE_ACCESS_KEY are not set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that part - we can still make this work (somehow ;-) )
But on travis I'd like to see an explicit job with TC=selenium listed
|
I've made the requested changes. Tests are passing on my travis instance: https://travis-ci.org/individual-it/owncloud-core/builds/196904627 |
this is okay - nothing to worry from my pov |
|
We need to fix the regular phpunit execution as done on jenkins. @PVince81 @SergioBertolinSG integration tests will stay in build/integration? What is the current play? Otherwise I suggest to restructure the tests folder:
objections? |
|
The even more basic question is were do you like to have the selenium tests run?
I've set it up on Travis cause it's all public, free of costs and easy to set up, but you also could run it on/through your Jenkins architecture
…On January 31, 2017 7:03:59 PM GMT+05:45, "Thomas Müller" ***@***.***> wrote:
We need to fix the regular phpunit execution as done on jenkins.
The selenium tests are executed as well.
Maybe we move the selenium tests into a different folder.
@PVince81 @SergioBertolinSG integration tests will stay in
build/integration? What is the current play?
Otherwise I suggest to restructure the tests folder:
- tests
- unit (move all unit tests in here)
- integration tests (move build/integration in here)
- ui (move all selenium tests are of this pr)
objections?
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#27057 (comment)
|
As far as I can tell: travis is fine for me - let's see how this evolves. We can always move these tests to jenkins. |
|
In any case: @individual-it thanks a lot for the effort you put into this! Much appreciated! |
|
I think we should use behat mink for this http://mink.behat.org/en/latest/ The tests which find the elements using xpath can break easily, that requires an improvement of the frontend to include proper ids to every element. |
|
I totally agree that relying on xpath might be problematic, depending on how you use it. use of page object pattern sounds very good to me. Selenium-documentation on Page Objects Its basically the question about how much to generalize and reuse the code. And more reuse of code if of course better. You can make mink drive selenium on sauce-labs. |
|
I've converted my Sauce Labs account into a OSS account and that makes the tests public. If someone is interested in seeing this test+result on sauce labs site here it is: https://saucelabs.com/beta/tests/53361d4a9812466a87027fdb5a4b0109/commands#0 |
Selenium better waiting & nicer code formatting
@DeepDiver1975 sounds good |
To be consistent with the current integration tests suite which are already using behat. Also easier to create new tests and understand them. |
|
+1 for behat |
|
@SergioBertolinSG I just watched your behat presentation on owncloud https://www.youtube.com/watch?v=_Sbgctq1P1U |
|
Closing this and will make a new PR with a behat / mink solution |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description
This is a slenium test for the PR #26979 that fixes the issue #26975
It depends on PR #27056 (Selenium implementation)
Related Issue
PR #26979
issue #26975
Motivation and Context
See issue #27055 & PR #27056
Types of changes
Checklist: