Convert more tests to modern testing style #2054
Conversation
clekstro
force-pushed
the
ce-convert-more-tests-contd
branch
from
9de57b7 to
e95504d
Compare
clekstro
force-pushed
the
ce-convert-more-tests-contd
branch
from
26722bb to
89d1f1d
Compare
This causes test flappiness when running all tests together
This also includes a new helper (credit to @ppcano here: emberjs/ember-test-helpers#332 (comment)) to work around emberjs/ember-test-helpers#332.
Hoping based on https://engineering.linkedin.com/blog/2018/01/ember-timer-leaks that it will help us identify some leakage that is causing some annoying test flappiness.
Was just an experiment, removing for now. This reverts commit 9e434c4.
This is now built-in as `waitFor` within @ember/test-helpers.
It appears that these tests are very flaky, so need to figure out a way to permanently fix them. I do not want them, however, to hold up this chunk of work.
This does lose a single assertion that we are calling the auth service correctly, but I would argue that this should be a unit test on the auth service if it is needed at all.
This was causing a super weird error regarding `window.wait`.
We no longer use `withFeature` but rather import `enableFeature` directly in any test that needs it
clekstro
force-pushed
the
ce-convert-more-tests-contd
branch
from
16898ce to
a0d9ee8
Compare
This will automatically sign a user out and clear browser state, ensuring every test starts off with the same state
|
This does skip a few tests that were giving trouble (mainly |
| test('app does not request feature flags on boot if available in local storage', function (assert) { | ||
| assert.expect(1); | ||
| test('app does not request feature flags on boot if available in local storage', async function (assert) { | ||
| assert.expect(1); |
There was a problem hiding this comment.
Not sure, but it seems like this test should expect 2 assertions, shouldn't it?
There was a problem hiding this comment.
I think this test is probably just a bit too clever. Instead of the normal assert.ok(true), we assert the opposite (assert.ok(false)) so that the test would fail if the request were ever made. This isolates the failure from someone accidentally deleting the expectation that a certain amount of assertions are made, but it's definitely not the most intuitive thing to read...
There was a problem hiding this comment.
Ah, that makes sense! I didn't get the idea, it just catched my eye so I thought to mention. Thanks for the clarification!
There was a problem hiding this comment.
@clekstro it's amazing! Thanks for taking care of this! I just noticed one tiny thing with expect, everything else looks great.
This already happens within `setupApplicationTest`.
I have left it in place for now (in case it has some use later), but we reset the local/session storage anyway before each acceptance test, and our only uses of it right now were essentially duplicating that work.
No description provided.