Skip to content
This repository has been archived by the owner on Jul 29, 2024. It is now read-only.

fix(Jasmine): Update Jasmine to support Node8 async/await #4608

Merged
merged 1 commit into from
Dec 1, 2017

Conversation

qiyigg
Copy link
Contributor

@qiyigg qiyigg commented Dec 1, 2017

fix(jasmine): Update Jasmine to support Node8 async/await
Breaking change for TypeScript:

JasmineWD doesn't know anything about async/await, turns off JasmineWD if control flow was disabled.

It will affect TypeScript tests that are using async/await and

  1. miss some await keyword in the test.(Previously, this might cause the
    test failed silently and be reported as pass), or
  2. use Promise in jasmine expect function

Before

await expect(getPromise()).toEqual(42);

After

expect(await getPromise()).toEqual(42);

@juliemr
Copy link
Member

juliemr commented Dec 1, 2017

Code LGTM! I'd suggest updating the commit message to have a clear 'before/after' for breaking changes, like this: 5034c89

@qiyigg
Copy link
Contributor Author

qiyigg commented Dec 1, 2017

Sure, will squash and rewrite the comment after fixing all breaks.

Breaking change for TypeScript:

JasmineWD doesn't know anything about async/await, turns off JasmineWD if control flow was disabled.

It will affect TypeScript tests that are using async/await and
1. miss some await keyword in the test.(Previously, this might cause the
test failed silently and be reported as pass), or
2. use Promise in jasmine expect function

Before
```ts
await expect(getPromise()).toEqual(42);
```

After
```ts
expect(await getPromise()).toEqual(42);
```
@rdbaron
Copy link

rdbaron commented Dec 8, 2017

Just to clarify:

"It will affect TypeScript tests that are using async/await and"

It will affect ANY tests that turn off the control flow, not just TypeScript tests, right?

Out of curiosity, what was broken? I recently migrated to async/await (I don't use TypeScript though) and my expect statements have been working as expected using:

await expect(getPromise()).toEqual(42)

@qiyigg
Copy link
Contributor Author

qiyigg commented Dec 8, 2017

It will affect ANY tests that turn off the control flow AND using async/await
Only TypeScript supports async/await before this change (you cannot use javascript async/await before this change); therefore it will only affect/break TypeScript

For JavaScript, if you write test like this : await expect(getPromise()).toEqual(42). it should throw an ugly error.
If you don't get that error, maybe

  1. you forgot to turn off control flow
  2. your getPromise() doesn't really return a promise.

@rdbaron
Copy link

rdbaron commented Dec 8, 2017

I'm sure I'm missing something obvious here, but I seem to have my 2000+ line repo running without issue using Protractor 5.2.0 (prior to this commit).

I am using SELENIUM_PROMISE_MANAGER: false

Here is a trivial example that seems to show everything working as I would expect:

let angularBanner = element(by.css('.hero')).$('h2')
let todoList = element.all(by.repeater('todo in todoList.todos'))

describe('async/await should', () => {
  it('work when Promises resolve', async () => {
    await browser.get('https://angularjs.org')
    await expect(angularBanner.getText()).toEqual('AngularJS')
    return expect(todoList.count()).toBe(2)
  })

  it('fail when expectations are incorrect', async () => {
    await expect(angularBanner.getText()).toEqual('SomeJunk')
    return expect(todoList.count()).toBe(0)
  })
})

@renehamburger
Copy link

@qiyigg, would you mind explaining briefly which issue this commit fixes? We've also been making use of the async expect as in await expect(getPromise()).toEqual(42); throughout, as it made our tests more readable.

@qiyigg
Copy link
Contributor Author

qiyigg commented Dec 12, 2017

@rdbaron hmm, that's also quite surprising for me. I double checked the JasmineWD github and found jasmineWD has already supported async/await in the new release, but we didn't have that change inside google and therefore we didn't notice async/await can be used directly externally.

@renehamburger the original idea was to disable jasmineWD when disable control flow since previously jasmineWD didn't support async/await and we thought jasmineWD shouldn't be existed if we don't use control flow. JasmineWD change the original behavior of Jasmine expect, so that it can accept promise.

"await expect(getPromise()).toEqual(42);" looks more readable: My personally understanding is when we make an assertion using "expect(a).toEqual(b)", a and b should be comparable. Only the process returns promise should be awaited. e.g. we don't actually wait for "button = element(by.css('button'))" (the locator is lazy loaded), but wait for button.getText() / button.click().

But anyway, this change was unnecessary at this point.
For now, we just revert the change and release a new version.

Sorry for the inconvenience.

qiyigg added a commit to qiyigg/protractor that referenced this pull request Dec 12, 2017
…gular#4608)"

This reverts commit 5d13b00.
This commit is unnecessary, revert this commit to avoid breaking changes
qiyigg added a commit that referenced this pull request Dec 12, 2017
)" (#4625)

This reverts commit 5d13b00.
This commit is unnecessary, revert this commit to avoid breaking changes
@renehamburger
Copy link

Thanks for the quick fix, @qiyigg!

@rdbaron
Copy link

rdbaron commented Dec 12, 2017

Thanks for the reply, @qiyigg.

Can you just clarify your previous comment for me though.

Only TypeScript supports async/await before this change (you cannot use javascript async/await before this change)

I am not using TypeScript and things seem to work. Is there something specific I should be aware of?

@qiyigg
Copy link
Contributor Author

qiyigg commented Dec 12, 2017

@rdbaron no, you are right. Your test should work directly. As I mentioned above, JasmineWD(a wrapper of Jasmine) has already fixed the bug so that it can support native async/await right now; therefore my PR was not needed any more and I have already reverted it. If you already changed your test to the new style, you don't need to change it back, it will work both with and without this PR.

firstor pushed a commit to firstor/protractor that referenced this pull request Apr 21, 2018
Breaking change for TypeScript:

JasmineWD doesn't know anything about async/await, turns off JasmineWD if control flow was disabled.

It will affect TypeScript tests that are using async/await and
1. miss some await keyword in the test.(Previously, this might cause the
test failed silently and be reported as pass), or
2. use Promise in jasmine expect function

Before
```ts
await expect(getPromise()).toEqual(42);
```

After
```ts
expect(await getPromise()).toEqual(42);
```
firstor pushed a commit to firstor/protractor that referenced this pull request Apr 21, 2018
…gular#4608)" (angular#4625)

This reverts commit 5d13b00.
This commit is unnecessary, revert this commit to avoid breaking changes
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants