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

Fix work with promise chains #568

Merged
merged 33 commits into from Jun 28, 2017

Conversation

Projects
None yet
2 participants
@APshenkin
Collaborator

APshenkin commented Jun 22, 2017

Our goals:

  • Fix #543 - After block not properly executed if Scenario fails
  • Expected behavior in promise chains: _beforeSuite hooks from helpers -> BeforeSuite from test -> _before hooks from helpers -> Before from test - > Test steps -> _failed hooks from helpers (if test failed) -> After from test -> _after hooks from helpers -> AfterSuite from test -> _afterSuite hook from helpers.
  • if during test we got errors from any hook (in test or in helper) - stop complete this suite and go to another
  • if during test we got error from Selenium server - stop complete this suite and go to another
  • if restart option is false - close all tabs expect one in _after (ready for webdriverIO, SeleniumWebdriver and Protractor. For Nightmare it will be hard to add (I found only this way https://github.com/rosshinkley/nightmare-window-manager)
  • Complete _after, _afterSuite hooks even After/AfterSuite from test was failed
  • Don't close browser between suites, when restart option is false. We should start browser only one time and close it only after all tests.
  • Close tabs and clear local storage, if keepCookies flag is enabled

Bonus:

  • Add step execution time in debug mode
  • Fix #548
@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Jun 22, 2017

Member

Please fix test/runner/interface_test.js, it tests the order of events

Member

DavertMik commented Jun 22, 2017

Please fix test/runner/interface_test.js, it tests the order of events

APshenkin added some commits Jun 23, 2017

@APshenkin APshenkin changed the title from Run after block after test fails to [WIP] Fix work with promise chains Jun 23, 2017

APshenkin added some commits Jun 23, 2017

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Jun 24, 2017

Member

Wow, awesome work. We are getting closer and closer to 1.0 release!

Member

DavertMik commented Jun 24, 2017

Wow, awesome work. We are getting closer and closer to 1.0 release!

@@ -45,6 +49,12 @@ module.exports = function (suite) {
suites.shift();
}
if (!opts) opts = {};
afterAllHooks = [];

This comment has been minimized.

@DavertMik

DavertMik Jun 25, 2017

Member

this was already defined at line 27

@DavertMik

DavertMik Jun 25, 2017

Member

this was already defined at line 27

This comment has been minimized.

@APshenkin

APshenkin Jun 26, 2017

Collaborator

fixed

@APshenkin

APshenkin Jun 26, 2017

Collaborator

fixed

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Jun 25, 2017

Member

Please try to use interactive shell to check that nothing was broken for it.
Also, execute one test with within block to be sure this changes to recorder doesn't break them.

Member

DavertMik commented Jun 25, 2017

Please try to use interactive shell to check that nothing was broken for it.
Also, execute one test with within block to be sure this changes to recorder doesn't break them.

@APshenkin

This comment has been minimized.

Show comment
Hide comment
@APshenkin

APshenkin Jun 25, 2017

Collaborator

@DavertMik
During these changes I found one interaction that may be not correct. I want to ask your advice about it.
Now we have clear promises chain as described in topic. It will break only after fail. Also if we get error in BeforeSuite, Before or After hooks we will skip all tests in this suite (looks it's ok, because there is no sense to try repeat test with the sameBeforeSuite/Before/After if it's not working). But there is an interesting ways with After and AfterSuite hooks.
If After hook from test will fail, then _after hooks from helpers will not be executed.
The same thing with AfterSuite - if it fails, then _afterSuite hooks from helpers will not be executed. It looks normal, but this can cause an issue, that after failure in these hooks, browser will not close or we will not clear environment for next tests.

I think, that maybe we can track this and force close/clear browser in this cases.

What do you think? Should we track this, or it should be resolved from user side?

Collaborator

APshenkin commented Jun 25, 2017

@DavertMik
During these changes I found one interaction that may be not correct. I want to ask your advice about it.
Now we have clear promises chain as described in topic. It will break only after fail. Also if we get error in BeforeSuite, Before or After hooks we will skip all tests in this suite (looks it's ok, because there is no sense to try repeat test with the sameBeforeSuite/Before/After if it's not working). But there is an interesting ways with After and AfterSuite hooks.
If After hook from test will fail, then _after hooks from helpers will not be executed.
The same thing with AfterSuite - if it fails, then _afterSuite hooks from helpers will not be executed. It looks normal, but this can cause an issue, that after failure in these hooks, browser will not close or we will not clear environment for next tests.

I think, that maybe we can track this and force close/clear browser in this cases.

What do you think? Should we track this, or it should be resolved from user side?

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Jun 25, 2017

Member

If After hook from test will fail, then _after hooks from helpers will not be executed.

Yes, this makes a big problem for closing browser sessions...
I have 2 ideas about this:

  1. run _after hook for Helper even test has failed in After. Same for AfterSuite. I think it is better to have all _after hooks to be executed no matter what happened.
  2. add new finalize hook.

But this is important to solve it in one way or another.

Member

DavertMik commented Jun 25, 2017

If After hook from test will fail, then _after hooks from helpers will not be executed.

Yes, this makes a big problem for closing browser sessions...
I have 2 ideas about this:

  1. run _after hook for Helper even test has failed in After. Same for AfterSuite. I think it is better to have all _after hooks to be executed no matter what happened.
  2. add new finalize hook.

But this is important to solve it in one way or another.

@APshenkin

This comment has been minimized.

Show comment
Hide comment
@APshenkin

APshenkin Jun 26, 2017

Collaborator

@DavertMik please review the changes. pause() and within block works as expected

Collaborator

APshenkin commented Jun 26, 2017

@DavertMik please review the changes. pause() and within block works as expected

APshenkin added some commits Jun 26, 2017

@APshenkin APshenkin changed the title from [WIP] Fix work with promise chains to Fix work with promise chains Jun 26, 2017

Show outdated Hide outdated lib/helper/SeleniumWebdriver.js
* I.closeTabsExceptForOne();
* ```
*/
closeTabsExceptForOne() {

This comment has been minimized.

@DavertMik

DavertMik Jun 27, 2017

Member

better to rename:

closeOtherTabs
@DavertMik

DavertMik Jun 27, 2017

Member

better to rename:

closeOtherTabs

This comment has been minimized.

@APshenkin

APshenkin Jun 27, 2017

Collaborator

fixed

@APshenkin

APshenkin Jun 27, 2017

Collaborator

fixed

@@ -22,6 +22,7 @@ class Step {
}
run() {
this.startTime = new Date();

This comment has been minimized.

@DavertMik

DavertMik Jun 27, 2017

Member

I think this date-time counter should be moved out to https://github.com/Codeception/CodeceptJS/blob/master/lib/listener/steps.js

@DavertMik

This comment has been minimized.

@APshenkin

APshenkin Jun 27, 2017

Collaborator

If we place it in steps.js then we can't pass this value properly between states. Or I don't know how to do it. If you have any suggestion, it will be awesome

@APshenkin

APshenkin Jun 27, 2017

Collaborator

If we place it in steps.js then we can't pass this value properly between states. Or I don't know how to do it. If you have any suggestion, it will be awesome

Show outdated Hide outdated lib/scenario.js
event.emit(event.test.before);
};
module.exports.teardown = function () {
if (!recorder.isRunning()) {

This comment has been minimized.

@DavertMik

DavertMik Jun 27, 2017

Member

you repeat this 3 lines too much.
better to make a method recorder->startUnlessRunning()

@DavertMik

DavertMik Jun 27, 2017

Member

you repeat this 3 lines too much.
better to make a method recorder->startUnlessRunning()

This comment has been minimized.

@APshenkin

APshenkin Jun 27, 2017

Collaborator

fixed

@APshenkin

APshenkin Jun 27, 2017

Collaborator

fixed

@@ -11,6 +11,14 @@ module.exports = {
passed: 'test.passed',
failed: 'test.failed',
},
hook: {

This comment has been minimized.

@DavertMik

DavertMik Jun 27, 2017

Member

This name is a bit confusing. Ok, these are special events fired by a failing after and afterSuite.

Ok, let's try to clarify their context. Those methods should not fail, so if they do it is an error.
So I propose to have next events: test.after.error, and suite.after.error instead.

@DavertMik

DavertMik Jun 27, 2017

Member

This name is a bit confusing. Ok, these are special events fired by a failing after and afterSuite.

Ok, let's try to clarify their context. Those methods should not fail, so if they do it is an error.
So I propose to have next events: test.after.error, and suite.after.error instead.

This comment has been minimized.

@APshenkin

APshenkin Jun 27, 2017

Collaborator

we can't use test.after/suite.after, because they are already defined. I suggest test.afterError, suite.afterError

@APshenkin

APshenkin Jun 27, 2017

Collaborator

we can't use test.after/suite.after, because they are already defined. I suggest test.afterError, suite.afterError

This comment has been minimized.

@DavertMik

DavertMik Jun 27, 2017

Member

test.afterError, suite.afterError

good

@DavertMik

DavertMik Jun 27, 2017

Member

test.afterError, suite.afterError

good

Show outdated Hide outdated lib/actor.js
val = step.run.apply(step, args);
val = step.run.apply(step, args).then(() => {
step.endTime = new Date();
output.log(`-> ${step.toString()} was executed in ${(step.endTime - step.startTime) / 1000} sec`);

This comment has been minimized.

@DavertMik

DavertMik Jun 27, 2017

Member

move this code to event listener

@DavertMik

DavertMik Jun 27, 2017

Member

move this code to event listener

Show outdated Hide outdated lib/interfaces/bdd.js
@@ -88,7 +95,18 @@ module.exports = function (suite) {
fn = opts;
opts = {};
}
if (!afterAllHooksAreLoaded) {
afterAllHooks.forEach(function (hook) {

This comment has been minimized.

@DavertMik

DavertMik Jun 27, 2017

Member

add some comments about what issues does it solve

@DavertMik

DavertMik Jun 27, 2017

Member

add some comments about what issues does it solve

This comment has been minimized.

@APshenkin

APshenkin Jun 27, 2017

Collaborator

I added comment in test.
FYI, we collect all hooks in right order in array and then push them to suite. We start collecting hooks from helpers and after from testfile. this cause hooks reordering
(expected: After from test -> _after hooks from helpers -> AfterSuite from test -> _afterSuite hook from helpers.
actual: _after hooks from helpers -> After from test -> _afterSuite hook from helpers. -> AfterSuite from test )

This change fix this chain

@APshenkin

APshenkin Jun 27, 2017

Collaborator

I added comment in test.
FYI, we collect all hooks in right order in array and then push them to suite. We start collecting hooks from helpers and after from testfile. this cause hooks reordering
(expected: After from test -> _after hooks from helpers -> AfterSuite from test -> _afterSuite hook from helpers.
actual: _after hooks from helpers -> After from test -> _afterSuite hook from helpers. -> AfterSuite from test )

This change fix this chain

@DavertMik DavertMik merged commit a79a725 into master Jun 28, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details

@DavertMik DavertMik deleted the use-after-when-fail branch Jun 28, 2017

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