chore(docs): new framework requirements in Protractor 6.0 #3893

sjelin · 2016-12-29T20:50:03Z

This will allow us to implement restartBrowserBetweenTests without the control flow, and in the future maybe add more features users have been asking for.

juliemr · 2016-12-29T20:54:27Z

See issue at #3894

juliemr · 2016-12-29T21:00:06Z

lib/frameworks/README.md

+- The returned promise must follow the same rules as before.
+
+Note that `testPass` and `testFail` events will be replaced by
+`runner.afterEach`, `runner.runTestPreparer` will be replaced by `beforeAll`,


Why change the API for runTestPreparer? This seems like unnecessary churn.

Just to make it more consistent

I don't think that's worth the braking change, if we aren't updating the emitters.

juliemr · 2016-12-29T21:01:47Z

lib/frameworks/README.md

+
+- The returned promise must follow the same rules as before.
+
+Note that `testPass` and `testFail` events will be replaced by


I think I'd prefer to make minimal changes here. I think we can keep the testPass and testFail events, since there are good and useful patterns for wanting to do event-based handling of those things without blocking the asynchronous flow. I think we should just add the afterEach (or beforeEach, not sure it really matters where we put the edge case).

We would still have the runner emit testPass and testFail, we would just do that in runner.afterEach. So instead of having the framework handle event emission we'd do it ourselves. And I think it's pretty redundant to have the frameworks do both.

Another potential problem: I think frameworks should treat afterEach as part of the test, so if that errors, it could potentially change the test outcome from pass to fail. And the framework might not even have access to pass/fail stats before it's fully done.

Ok, I think you're right

juliemr · 2016-12-29T21:02:22Z

lib/frameworks/README.md

+
+Frameworks following the old specification will still work until at least
+Protractor 7.0.  If you write a backwards-compatible framework, Protractor will
+automatically detect that and handle deduplication.  However, do not *partially*


If we go with my comments above and only add the afterEach function, then we don't need this warning.

juliemr · 2016-12-29T21:11:18Z

lib/frameworks/README.md

+In Protractor 6.0, the requirements will be changed to the following:
+
+- `runner.afterEach` will have to be called after each test finishes.  It must
+  be called with a `testInfo` object of the following structure:


Should mention that this needs to wait on a returned promise.

sjelin · 2016-12-29T23:23:10Z

@juliemr All comments addressed, but you're going to need to take a look again 😛

Also converted the code in `lib/frameworks/README.md` to typescript. Also exported the type of `Runner` so that framework-writers can use typescript. Closes angular#3893

juliemr · 2016-12-29T23:38:53Z

lib/frameworks/README.md

+  optionally a `specResults` object of the following structure:
+
+  ```ts
+  specResults: Array<{


I don't think the switch to Array makes this more readable.

I was trying to make it an actual typescript type. But I'll change it back, since this isn't real code anyway.

juliemr · 2016-12-29T23:40:48Z

lib/frameworks/README.md

+  If your framework does not export a `protractorVersion` property which is at
+  least 6.0.0, and `testPass` or `testFail` are emitted before
+  `runner.afterEach` is called, Protractor will assume your framework is old and
+  does not support `runner.afterEach`.  In this case, Protractor will call


Why don't we just check for presence of afterEach on the object? This seems simpler than requiring a version export.

The framework calls us, not the other way around. There's no way to check that they plan on calling us

We could also add a requirement that they call afterEach before they emit testPass or testFail. Jasmine and Mocha will certainly do this. But I'm concerned that this might be difficult for other frameworks.

Ah of course, my bad. Asking for a version still feels like a very blunt solution though - we could instead warn if afterEach isn't being called at all (might not get real-time errors, but we could certainly warn if, say, afterEach wasn't called during an entire run)

So let's say we see a testPass event but haven't seen afterEach yet and restartBrowserBetweenTests is on. Do we assume it's an old framework and call browser.restart now? Do we wait in hopes that afterEach will be called in the future?

I'd say we log a warning if we see that behavior and move on without trying to fix it. Too harsh?

I guess it's fine. So we'd just break restartBrowserBetweenTests on old frameworks then?

Right. And obviously we'll update frameworks that we control before we make the breaking change.

Also converted the code in `lib/frameworks/README.md` to typescript. Also exported the type of `Runner` so that framework-writers can use typescript. Part of angular#3893

sjelin · 2016-12-30T03:43:30Z

@juliemr all comments addressed.

juliemr · 2016-12-30T03:56:08Z

LGTM!

…

On Thu, Dec 29, 2016 at 7:43 PM, Sammy Jelin ***@***.***> wrote: @juliemr <https://github.com/juliemr> all comments addressed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3893 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB_DrRirZlP9_bQwdspUDGuENYYx6-Icks5rNH3kgaJpZM4LX1kT> .

Follow up to angular#3893. Part of angular#3893

Follow up to #3893. Part of #3893

Follow up to angular#3893. Part of angular#3893

Follow up to angular/protractor#3893. Part of #3893

sjelin added the pr: needs review label Dec 29, 2016

sjelin assigned juliemr and sjelin Dec 29, 2016

sjelin requested a review from juliemr December 29, 2016 20:50

googlebot added cla: yes labels Dec 29, 2016

sjelin added this to the 5.0 milestone Dec 29, 2016

juliemr suggested changes Dec 29, 2016

View reviewed changes

sjelin force-pushed the newfw branch 2 times, most recently from aaba4fa to e0e278b Compare December 29, 2016 23:22

sjelin force-pushed the newfw branch from e0e278b to cb56767 Compare December 29, 2016 23:27

juliemr suggested changes Dec 29, 2016

View reviewed changes

sjelin force-pushed the newfw branch from cb56767 to 4aefa23 Compare December 30, 2016 00:10

sjelin force-pushed the newfw branch from 4aefa23 to 9553155 Compare December 30, 2016 00:11

chore(docs): new framework requirements in Protractor 6.0

6cc2147

Also converted the code in `lib/frameworks/README.md` to typescript. Also exported the type of `Runner` so that framework-writers can use typescript. Part of angular#3893

sjelin force-pushed the newfw branch from 9553155 to 6cc2147 Compare December 30, 2016 03:42

sjelin merged commit c194af8 into angular:beta Dec 30, 2016

sjelin added a commit to sjelin/protractor that referenced this pull request Dec 30, 2016

chore(frameworks): allow frameworks to call afterEach.

e2f74e2

Follow up to angular#3893. Part of angular#3893

sjelin added a commit to sjelin/protractor that referenced this pull request Dec 30, 2016

chore(frameworks): allow frameworks to call afterEach.

8f83218

Follow up to angular#3893. Part of angular#3893

This was referenced Dec 30, 2016

chore(frameworks): allow frameworks to call afterEach. #3906

Merged

Support runner.afterEach in jasmine adapter #3908

Closed

Support runner.afterEach in mocha adapter #3909

Closed

sjelin added a commit to sjelin/protractor that referenced this pull request Jan 3, 2017

chore(frameworks): allow frameworks to call afterEach.

e5c4684

Follow up to angular#3893. Part of angular#3893

sjelin added a commit to sjelin/protractor that referenced this pull request Jan 18, 2017

chore(frameworks): allow frameworks to call afterEach.

3397c3a

Follow up to angular#3893. Part of angular#3893

sjelin added a commit that referenced this pull request Jan 18, 2017

chore(frameworks): allow frameworks to call afterEach. (#3906)

d868ab9

Follow up to #3893. Part of #3893

sjelin added a commit that referenced this pull request Jan 18, 2017

chore(frameworks): allow frameworks to call afterEach. (#3906)

23bc943

Follow up to #3893. Part of #3893

igniteram pushed a commit to igniteram/protractor that referenced this pull request Feb 21, 2017

chore(frameworks): allow frameworks to call afterEach. (angular#3906)

3f4050f

Follow up to angular#3893. Part of angular#3893

bodyduardU pushed a commit to bodyduardU/protractor that referenced this pull request Dec 5, 2022

chore(frameworks): allow frameworks to call afterEach. (#3906)

d1da439

Follow up to angular/protractor#3893. Part of #3893

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(docs): new framework requirements in Protractor 6.0 #3893

chore(docs): new framework requirements in Protractor 6.0 #3893

sjelin commented Dec 29, 2016

juliemr commented Dec 29, 2016

juliemr Dec 29, 2016

sjelin Dec 29, 2016

juliemr Dec 29, 2016

juliemr Dec 29, 2016

sjelin Dec 29, 2016

juliemr Dec 29, 2016

sjelin Dec 29, 2016

juliemr Dec 29, 2016

juliemr Dec 29, 2016

sjelin commented Dec 29, 2016

juliemr Dec 29, 2016

sjelin Dec 30, 2016

sjelin Dec 30, 2016

juliemr Dec 29, 2016

sjelin Dec 30, 2016

sjelin Dec 30, 2016

juliemr Dec 30, 2016

sjelin Dec 30, 2016

juliemr Dec 30, 2016

sjelin Dec 30, 2016

juliemr Dec 30, 2016

sjelin commented Dec 30, 2016

juliemr commented Dec 30, 2016 via email


		- The returned promise must follow the same rules as before.

		Note that `testPass` and `testFail` events will be replaced by

chore(docs): new framework requirements in Protractor 6.0 #3893

chore(docs): new framework requirements in Protractor 6.0 #3893

Conversation

sjelin commented Dec 29, 2016

juliemr commented Dec 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjelin commented Dec 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjelin commented Dec 30, 2016

juliemr commented Dec 30, 2016 via email