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

//Add some tests on delivery options #4215

Merged

Conversation

@maximebiloe
Copy link
Contributor

commented Oct 7, 2015

No description provided.

@maximebiloe maximebiloe force-pushed the maximebiloe:starter-theme/feat/tests branch Oct 7, 2015

@djfm

View changes

tests/StarterTheme/specs/opc.js Outdated
@@ -12,6 +12,72 @@ function toggleAllTermsCheckboxes () {
}

describe('The One Page Checkout', function () {

before(function () {
return browser.url('/').click('.menu a').click('a[data-link-action="add-to-cart"]');

This comment has been minimized.

Copy link
@djfm

djfm Oct 8, 2015

Contributor

Please add a short pause after clicking the add-to-cart link: since adding to the cart is done in AJAX, sometimes when the actual test starts I don't have a product in the cart yet. Reloading the page might also do the trick.

@djfm

View changes

tests/StarterTheme/specs/opc.js Outdated
browser.getAttribute('#delivery-method input:checked', 'id').then(function (firstattr) {
browser.refresh().then(function () {
browser.getAttribute('#delivery-method input:checked', 'id').then(function (secondattr) {
secondattr.should.equal(firstattr);

This comment has been minimized.

Copy link
@djfm

djfm Oct 8, 2015

Contributor

If I replace this line with: secondattr.should.equal('lalala'); the test still passes, so it probably doesn't test anything interesting :)

The reason why it doesn't fail when it should is because you are breaking the promise chain. Whenever you pass a callback to then, and you use a callback inside the first one, your first callback should return the promise returned by the second callback, like this:

        return browser.click('#delivery-method input:not(:checked)').then(function () {
          return browser.getAttribute('#delivery-method input:checked', 'id').then(function (firstattr) {
            return browser.refresh().then(function () {
              return browser.getAttribute('#delivery-method input:checked', 'id').then(function (secondattr) {
                secondattr.should.equal('lalala'); // will wail, as expected!
              });
            });
          });
        });

Now you're probably going to say: this is ugly to read and write! And that's true.

But fear not, it can be rewritten in a more readable way exploiting the webdriver.io method chaining and some refactoring, like this for example:

        var firstDeliveryMethodSelected;
        return browser
          .click('#delivery-method input:not(:checked)')
          .getAttribute('#delivery-method input:checked', 'id')
          .then(function (deliveryMethodSelected) {
            firstDeliveryMethodSelected = deliveryMethodSelected;
          })
          .refresh()
          .getAttribute('#delivery-method input:checked', 'id')
          .then(function (deliveryMethodSelected) {
            deliveryMethodSelected.should.equal(firstDeliveryMethodSelected);
          })
        ;

Can you do the same with the other tests? I.e. check that they do test something by changing the assertion slightly and checking that it fails, then refactor when there are too many nested callbacks.

@djfm

View changes

tests/StarterTheme/specs/opc.js Outdated
});

it('should remember carrier selected after reload', function () {
return browser.click('#delivery-method input:not(:checked)').then(function () {

This comment has been minimized.

Copy link
@djfm

djfm Oct 8, 2015

Contributor

Previous comment applies to this too.

@maximebiloe maximebiloe force-pushed the maximebiloe:starter-theme/feat/tests branch Oct 9, 2015

@maximebiloe maximebiloe force-pushed the maximebiloe:starter-theme/feat/tests branch 2 times, most recently Oct 19, 2015

@julienbourdeau julienbourdeau force-pushed the PrestaShop:feat/starter-theme branch 5 times, most recently to 9ae3cc4 Oct 27, 2015

@djfm djfm force-pushed the PrestaShop:feat/starter-theme branch from d766755 to a045ddb Nov 5, 2015

@maximebiloe maximebiloe force-pushed the PrestaShop:feat/starter-theme branch Nov 10, 2015

@djfm djfm force-pushed the PrestaShop:feat/starter-theme branch 2 times, most recently to 77a7fac Nov 10, 2015

@maximebiloe maximebiloe force-pushed the maximebiloe:starter-theme/feat/tests branch to 114f66b Nov 13, 2015

djfm added a commit that referenced this pull request Nov 13, 2015
Merge pull request #4215 from maximebiloe/starter-theme/feat/tests
//Add some tests on delivery options

@djfm djfm merged commit 9d1f700 into PrestaShop:feat/starter-theme Nov 13, 2015

1 check passed

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

@maximebiloe maximebiloe deleted the maximebiloe:starter-theme/feat/tests branch Feb 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.