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

Add the Selenium tests #73

Merged
merged 23 commits into from Sep 26, 2016

Conversation

Projects
None yet
2 participants
@iamakulov
Copy link
Collaborator

iamakulov commented Jul 2, 2016

This a pull request for automated Likely testing. As a goal, we want to build a functional test suite that will check the primary functionality such as sharing, working with History API, retrieving counters, etc. This will allow us to make changes easier and more reliable.

This PR’s build could be broken when I get to configuring Travis, so don’t mind this.

Related to #31.

@vitkarpov

iamakulov added some commits Jul 2, 2016

Configure the Sause Labs travis add-on
Sause Labs provide the Selenium testing as a service

@iamakulov iamakulov referenced this pull request Jul 2, 2016

Open

Long-term project improvements #31

5 of 9 tasks complete
@iamakulov

This comment has been minimized.

Copy link
Collaborator

iamakulov commented Jul 2, 2016

Looking for any suggestions about the planned tests (index.js). Are these enough? Should we also test something else or not test something? Any comments are appreciated.

@vitkarpov

This comment has been minimized.

Copy link
Collaborator

vitkarpov commented Jul 3, 2016

Seems really good! Maybe we should also add a kind of bugs section?

describe('bugs', function() {
  it('[#67] should get a correct title when the script is placed after the title element')
})

We can add related issues to tests, so we're sure they will not get back in the future (after refactoring and so on).

@iamakulov

This comment has been minimized.

Copy link
Collaborator

iamakulov commented Jul 3, 2016

Good point! Will add this.

@vitkarpov

This comment has been minimized.

Copy link
Collaborator

vitkarpov commented Jul 3, 2016

👍

This was referenced Jul 20, 2016

iamakulov added some commits Aug 29, 2016

Add the `integration-tests` task
And make the `test` task run it together with codestyle checking
@iamakulov

This comment has been minimized.

Copy link
Collaborator

iamakulov commented Aug 29, 2016

Implemented the first part of tests.

@vitkarpov

return expect(driver.findElement({css: '.likely'}).getAttribute('class')).to.eventually.contain('likely_visible likely_ready');
});

it('should fetch the counters for Facebook', function () {

This comment has been minimized.

@iamakulov

iamakulov Aug 29, 2016

Collaborator

This fails in almost any launches: Facebook returns the same API error as in #88.

This comment has been minimized.

@vitkarpov

vitkarpov Aug 29, 2016

Collaborator

I think we should mock externals services, use fake xhr or something. There's a couple of reasons for that:

  • you never know exactly how much time different requests need to finish, so the test could finish before and fails (delay)
  • we can't control external services, so actually we don't need to test their real answers: tests need for our code and should check itself only

This comment has been minimized.

@iamakulov

iamakulov Aug 29, 2016

Collaborator

Hm, need to think. One point for not mocking though: we can catch API changes we’re not aware about (such as the recent Facebook one).

This comment has been minimized.

@vitkarpov

vitkarpov Aug 29, 2016

Collaborator

I think it happens not so often, and it could be broken anyway, by third part, with tests or without.

This comment has been minimized.

@vitkarpov

vitkarpov Aug 29, 2016

Collaborator

Tests are needed to catch some errors before code goes in prod, in such case we can't control it anyway.

return expectToNotBeEmpty(driver, '.likely__counter_facebook');
});

it('should fetch the counters for Google+', function () {

This comment has been minimized.

@iamakulov

iamakulov Aug 29, 2016

Collaborator

This fails sometimes, haven’t investigated into the reason yet.

This comment has been minimized.

@vitkarpov

vitkarpov Aug 29, 2016

Collaborator

could relate to the first point in the comment above

return expect(driver.findElement({css: selector}).getText()).to.eventually.not.be.empty
}

function expectClickToOpen(driver, clickTargetSelector, windowUrlRegex) {

This comment has been minimized.

@iamakulov

iamakulov Aug 29, 2016

Collaborator

This is quite messy. If you have ideas how to improve this, feel free to do.

@iamakulov

This comment has been minimized.

Copy link
Collaborator

iamakulov commented Aug 29, 2016

The CI is currently failing as we haven’t properly configured the Sause Labs integration.

Add support for mocking the counter method
This solution is quite dirty, but I don’t know a better way to do this which is cheap enough.

iamakulov added a commit that referenced this pull request Sep 3, 2016

iamakulov added a commit that referenced this pull request Sep 3, 2016

Use the counter mocking
Reasoning: #73 (comment).
As the counters are mocked, there’s no need in using the canonical link.
@vitkarpov

This comment has been minimized.

Copy link
Collaborator

vitkarpov commented Sep 4, 2016

mocking seems okay anyway 👍

iamakulov added some commits Sep 4, 2016

Replace initialization timeout and test with Selenium’s `wait`
The test is unnecessary because `before` will fail anyway if the Likely isn’t properly initialized.
The `waitUntilInitialized` parameter will be actually used in the future commits.
});

it('should open the sharing dialog for Google+', function () {
this.timeout(10000);

This comment has been minimized.

@vitkarpov

vitkarpov Sep 5, 2016

Collaborator

maybe this set of its could be grouped together?

describe('should open the sharing dialog for', {
  before(function () {
    this.timeout(10000);
  })
  it('VK', function () { ... })
  it('Facebook', function () { ... })
  ...
})

This comment has been minimized.

@vitkarpov

vitkarpov Sep 5, 2016

Collaborator

also seems it could be automated a little bit:

[{ id: 'vk', urlReg: /vk\.com/ }, { ... }, ...].forEach(({ id, urlReg }) => {
  it(id, function () {
    return expectClickToOpen(driver, `.likely__widget_${id}`, urlReg);
  })
})

This comment has been minimized.

@vitkarpov

vitkarpov Sep 5, 2016

Collaborator

So the essential part is an array, and it looks much smaller

This comment has been minimized.

@iamakulov

iamakulov Sep 12, 2016

Collaborator

maybe this set of its could be grouped together?

also seems it could be automated a little bit:

Yep, let’s do this.


before(function () {
// The browser could start long
this.timeout(20000);

This comment has been minimized.

@vitkarpov

vitkarpov Sep 5, 2016

Collaborator

This constants seem like magic numbers, does selenium have API to wait the document loading?

This comment has been minimized.

@iamakulov

iamakulov Sep 12, 2016

Collaborator

It has, but this timeout is for Mocha, not for Selenium. It tells Mocha to stop the test only after 20 seconds instead of default 3. Mocha doesn’t know what’s running inside of the test and therefore can’t know if Selenium has already started or not.

This comment has been minimized.

@iamakulov

iamakulov Sep 12, 2016

Collaborator

I’ll move the timeout into a variable.

iamakulov added some commits Sep 12, 2016

Split the first test suite, generate tests, unify timeouts
* Split the first test suite into two ones because it actually consists of two different test groups
* Generate the tests for the newly created test suites instead of repeating the code
* Declare the single timeout of 20 s instead of specifying it here and there
@iamakulov

This comment has been minimized.

Copy link
Collaborator

iamakulov commented Sep 21, 2016

The tests are done now, the only thing left is to configure the CI to run them.

@vitkarpov
Copy link
Collaborator

vitkarpov left a comment

Cool! 👍

iamakulov added some commits Sep 23, 2016

Rewrite expectClickToOpen()
* Remove the unnecessary Promise nesting
* Close the dialog and switch back to the main window before comparing the URL
* Make the method work with Firefox
Switch to Firefox instead of Chrome
This is done to make the Travis configuration easier + keep the container-based environment (https://docs.travis-ci.com/user/ci-environment/#Virtualization-environments)
Update the codestyle
- Fix the ESLint errors.
- Re-configure two too tight rules:
  - `no-else-return` is rather bad (https://blog.mozilla.org/nnethercote/2009/08/31/no-else-after-return-considered-harmful/ expresses my ideas)
  - it’s OK to define the functions after their usage, so `no-use-before-define` now allows it.
Specify the Firefox version in .travis.yml
Looks like Travis provides an old Firefox version by default, which causes a mozilla/geckodriver#85 error
Decrease the Firefox version
50.0 is not yet available to download
@iamakulov

This comment has been minimized.

Copy link
Collaborator

iamakulov commented Sep 24, 2016

Finished the PR.

@vitkarpov, your feedback on a couple of points (see the code review) would be useful!

@iamakulov iamakulov changed the title [WIP] Add the Selenium tests Add the Selenium tests Sep 24, 2016

// If we do it before and the comparison fails, all the `.then()` branches won’t execute,
// and Selenium will continue working in the opened dialog.
// This will make all the following tests fail
.then(() => {

This comment has been minimized.

@iamakulov

iamakulov Sep 26, 2016

Collaborator

Does these comments (here and above in this function) look clear?

// because this function is executed right when Likely is loaded.
// There’s currently no way to do `likely.__counterMock = ...`
// before running this method.
options.counter = window.__likelyCounterMock || options.counter || counter;

This comment has been minimized.

@iamakulov

iamakulov Sep 26, 2016

Collaborator

Is this comment also clear?

@iamakulov iamakulov merged commit a34ebbe into master Sep 26, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@iamakulov iamakulov deleted the selenium-tests branch Sep 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment