Skip to content

testrunner: make environment a simple class #2769

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.

Sign up for GitHub

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

Jump to bottom
Merged
aslushnikov merged 10 commits into from
Jun 30, 2020
Merged

testrunner: make environment a simple class #2769

aslushnikov merged 10 commits into from
Jun 30, 2020

Conversation

@aslushnikov
Copy link
Contributor

@aslushnikov aslushnikov commented Jun 30, 2020 •
edited
Loading

This patch:

  • makes environment a simple class with optional methods beforeEach, afterEach, beforeAll, afterAll, globalSetup and globalTeardown
  • removes capability to have multiple hooks of the same name inside suite
  • removes default environment for test. (dit now adds a TraceTestEnvironment to the test)
  • extracts all environments that we use in our tests in //test/environments.js

Downsides:

  • we no longer know hook locations for the environments. This, however, should not be a big deal since stack traces (if any) will still point into it.
  • this also regresses hook locations for suites for simplicity. We can get them back, but it shouldn't be pressing since we now have only one hook of each kind in every suite.

@aslushnikov aslushnikov requested a review from dgozman June 30, 2020 08:11
dgozman
dgozman approved these changes Jun 30, 2020
View reviewed changes
test/environments.js
@@ -0,0 +1,223 @@
const utils = require('./utils');
Copy link
Contributor

@dgozman dgozman Jun 30, 2020

Choose a reason for hiding this comment

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

Missing copyright.

Copy link
Contributor Author

@aslushnikov aslushnikov Jun 30, 2020

Choose a reason for hiding this comment

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

Done

test/test.config.js Outdated
serverEnvironment.beforeAll(async state => {
const assetsPath = path.join(__dirname, 'assets');
const cachedPath = path.join(__dirname, 'assets', 'cached');
const extraLogger = utils.createTestLogger(valueFromEnv('DEBUGP', false), null, 'extra');
Copy link
Contributor

@dgozman dgozman Jun 30, 2020

Choose a reason for hiding this comment

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

This should be one logger per worker thread.

Copy link
Contributor Author

@aslushnikov aslushnikov Jun 30, 2020

Choose a reason for hiding this comment

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

Done.

test/test.js Outdated
state.page = null;
});
const logger = utils.createTestLogger(config.dumpLogOnFailure);
const browserEnvironment = new BrowserEnvironment(browserType, launchOptions, logger);
Copy link
Contributor

@dgozman dgozman Jun 30, 2020

Choose a reason for hiding this comment

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

Should BrowserEnvironment create the logger itself?

Copy link
Contributor Author

@aslushnikov aslushnikov Jun 30, 2020

Choose a reason for hiding this comment

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

Done.

test/environments.js
}
}

// simulate globalSetup per browserType that happens only once regardless of TestWorker.
Copy link
Contributor

@dgozman dgozman Jun 30, 2020

Choose a reason for hiding this comment

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

Let's use globalSetup!

Copy link
Contributor Author

@aslushnikov aslushnikov Jun 30, 2020

Choose a reason for hiding this comment

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

This will go away in the next patch - with first-class artifacts support in test runner.

@aslushnikov aslushnikov merged commit 1605cb4 into microsoft:master Jun 30, 2020
@aslushnikov aslushnikov deleted the attempt-to-extract-all-environments-into-separate-class branch June 30, 2020 23:51
pavelfeldman added a commit to pavelfeldman/playwright that referenced this pull request Jul 1, 2020
@pavelfeldman
revert: testrunner: make environment a simple class (microsoft#2769)
f0a4037
pavelfeldman added a commit to pavelfeldman/playwright that referenced this pull request Jul 1, 2020
@pavelfeldman
revert: testrunner: make environment a simple class (microsoft#2769)
5b6236f
pavelfeldman added a commit that referenced this pull request Jul 1, 2020
@pavelfeldman
revert: testrunner: make environment a simple class (#2769) (#2790)
e467ea5
aslushnikov added a commit to aslushnikov/playwright that referenced this pull request Jul 2, 2020
@aslushnikov
reland: testrunner: make environment a simple class
272e005 
This re-lands PR microsoft#2769

It was reverted before in microsoft#2790
because it was breaking the new CHANNEL bot.
aslushnikov added a commit to aslushnikov/playwright that referenced this pull request Jul 2, 2020
@aslushnikov
reland: testrunner: make environment a simple class
c9c64dd 
This re-lands PR microsoft#2769

It was reverted before in microsoft#2790
because it was breaking the new CHANNEL bot.
@aslushnikov aslushnikov mentioned this pull request Jul 2, 2020
Merged
aslushnikov added a commit that referenced this pull request Jul 2, 2020
@aslushnikov
reland: testrunner: make environment a simple class (#2812)
05b019f 
This re-lands PR #2769

It was reverted before in #2790
because it was breaking the new CHANNEL bot.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dgozman dgozman dgozman approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aslushnikov @dgozman