2316 refactor mocha tests to qunit #2330

candunaj · 2021-10-05T11:39:11Z

Removed mocha. Added Qunit. Refactored unit tests.

BobrImperator · 2021-10-05T11:49:33Z

packages/ember-simple-auth/tests/index.html

- {{content-for "test-body-footer"}}
- </body>
-</html>
+


are changes in this file necessary?

Change in this file fixed route /tests when you run it with "ember s", so I believe that the change in this file is valid.

Now I see, qunit needs some element it can hook into, so we should probably just go back to the old index.html file and add this below test-body block:

<div id="qunit"></div> <div id="qunit-fixture"> <div id="ember-testing-container"> <div id="ember-testing"></div> </div> </div>

BobrImperator · 2021-10-05T11:49:56Z

packages/ember-simple-auth/package.json

 "@glimmer/component": "^1.0.0",
 "@glimmer/tracking": "^1.0.0",
 "babel-eslint": "^10.1.0",
 "broccoli-asset-rev": "^3.0.0",
- "chai": "^4.1.0",
+ "chai": "^4.3.4",


chai is a part of mocha, so we want to remove it as well.

BobrImperator · 2021-10-05T11:51:45Z

packages/ember-simple-auth/package.json

@@ -31,11 +31,13 @@
 },
 "devDependencies": {
 "@ember/optional-features": "^1.3.0",
+ "@ember/test-helpers": "^2.4.2",
+ "@embroider/test-setup": "^0.43.5",


Embroider test setup is not needed for the migration

BobrImperator · 2021-10-05T11:52:04Z

packages/ember-simple-auth/package.json

 "@glimmer/component": "^1.0.0",
 "@glimmer/tracking": "^1.0.0",
 "babel-eslint": "^10.1.0",
 "broccoli-asset-rev": "^3.0.0",
- "chai": "^4.1.0",
+ "chai": "^4.3.4",
 "ember-cli": "~3.18.0",
 "ember-cli-blueprint-test-helpers": "^0.19.2",
 "ember-cli-chai": "^0.5.0",


chai ✂️

marcoow · 2021-10-20T13:42:08Z

🎉 Yay!

Could we implement the strategy for getting CI green that we discussed first though so we can merge this with a green CI?

candunaj · 2021-10-20T13:50:39Z

Hi, I have two branches. 1. 2316-refactor-mocha-tests-to-qunit CI green from ember-3.0 to ember-lts-3.24 2. ember-4 - CI green from ember-3.8 to ember-canary I believe that I should only remove ember-beta and ember-canary from the first branch and then everything will be green. What do you think?

…

On Wed, 20 Oct 2021 at 15:42, Marco Otte-Witte ***@***.***> wrote: 🎉 Yay! Could we implement the strategy for getting CI green that we discussed first though so we can merge this with a green CI? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2330 (comment)>, or unsubscribe <https:/notifications/unsubscribe-auth/AFT6GQB4XO24CL7KVON7A7DUH3BLVANCNFSM5FLSHR6Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

-- S pozdravom Stanislav Dunajčan e-mail: ***@***.***

marcoow · 2021-10-20T14:30:55Z

@candunaj so in our call recently we talked about the strategy for making things work with the current Torii situation. Can we make a separate PR for implementing that first? We can't just not run tests for ember-beta to "fix" CI :)

marcoow · 2021-10-29T09:15:46Z

@candunaj can we move 8702ed2 into its own PR?

marcoow

Very nice work 🙌

I found some things that I think still should be addressed but I'm looking forward getting this merged!

One general thing I recognized is that this replaces the nested describe blocks with a flat structure with just one module and tests nested directly into that (at least in some of the test files).Can we use nested modules instead? I feel a flat list of tests is much harder to navigate (plus, in a nested structure it's much easier to build up preconditions for tests in nested blocks incrementally).

marcoow · 2021-11-12T14:39:47Z

.github/workflows/ci.yml

@@ -56,8 +56,6 @@ jobs:
 - classic-test-app
 - test-app
 test-suite:
- - test:one ember-3.0
- - test:one ember-lts-3.4


Why does this change? Can we not support 3.0 and 3.4 when using QUnit?

Sorry, I wanted to write ember-qunit 5. ember-qunit 5 supports ember 3.8+.
https:/emberjs/ember-qunit

Can we use a version of ember-qunit that supports Ember 3.0 as well? I'd rather upgrade to ember-qunit 5 (and drop support for Ember < 3.8) in a separate PR then.

Yes, I can use ember-qunit 4 with some change in the source code. I will do it.

marcoow · 2021-11-12T14:40:10Z

packages/classic-test-app/package.json

@@ -70,4 +68,4 @@
 "cssstyle": "~1.2.2",
 "jsdom": "^11.10.0"
 }
-}
+}


Suggested change

}

}

this seems unrelated anyway

packages/classic-test-app/tests/.eslintrc.js

marcoow · 2021-11-12T14:43:04Z

packages/ember-simple-auth/.eslintrc.js

+ // test files
+ files: ['tests/**/*-test.{js,ts}'],
+ extends: ['plugin:qunit/recommended'],
+ },


See above – I assume this should go into the config that previously contained the Mocha linting settings?

marcoow · 2021-11-12T14:43:15Z

packages/ember-simple-auth/addon/services/session.js

@@ -1,4 +1,4 @@
-import { alias, oneWay } from '@ember/object/computed';
+import { alias, readOnly } from '@ember/object/computed';


This seems unrelated?

There are 2 unit tests which expect three properties in session service to be readOnly and fail when set. So I have changed properties isAuthenticated, data and store to readOnly.

But oneWay should be the same? My point is migrating the tests from Mocha to Qunit shouldn't require any changes in the tested code?

oneWay will not throw an error when set. I have found the bug in mocha tests so the bug was always there.

it('is read-only', function() { expect(() => { sessionService.set('store', 'some other store'); }).to.throw; });

it should be

}).to.throw();

Change isAuthenticated from oneWay to readOnly has fixed 3 tests, but it can be considered as breaking change. So maybe we should remove these tests?

Hm, we should definitely move that into a separate PR to not also mix actual changes to the behavior into this – it's already a huge PR (necessarily) so I'd keep it as limited as possible :)

marcoow · 2021-11-12T14:57:03Z

packages/ember-simple-auth/tests/unit/session-stores/shared/storage-event-handler-behavior.js

@@ -1,52 +1,51 @@
+


Suggested change

marcoow · 2021-11-12T14:57:47Z

packages/test-app/package.json

@@ -73,4 +71,4 @@
 "cssstyle": "~1.2.2",
 "jsdom": "^11.10.0"
 }
-}
+}


Suggested change

}

}

marcoow · 2021-11-12T14:58:01Z

packages/test-app/config/fastboot-testing.js

+ // eslint-disable-next-line node/no-missing-require
+ najax = require('najax');
+} catch (err) {
+ // ignore the error


Why is this error ignored?

packages/test-app/tests/.eslintrc.js

marcoow · 2021-11-12T14:59:16Z

packages/test-app/tests/index.html

+ {{content-for "test-body-footer"}}
+</body>
+
+</html>


Seems like this diff of this file is a bit messed up because of the reformatting (and now missing indentation)?

BobrImperator · 2021-11-25T12:57:41Z

I went through the changes and there isn't anything from my side apart from what Marco's pointed out.
Although, I'm not sure if we were supposed to change test and classic-testapps as well, thought that just ESA package refactor is enough.

Though again, it probably makes sense as it's what we need to do for Ember 4.0 anyway.

marcoow · 2021-11-30T08:00:08Z

packages/classic-test-app/package.json

@@ -14,24 +14,22 @@
 "test:fastboot": "ember fastboot:test"
 },
 "devDependencies": {
+ "@ember/jquery": "^2.0.0",


Is this related?

marcoow · 2021-11-30T08:04:16Z

packages/ember-simple-auth/tests/unit/authenticators/oauth2-password-grant-test.js

- 'token_type_hint': 'refresh_token',
- 'token': 'refresh token!'
- });
+ if (body.token_type_hint === 'refresh_token') {


@candunaj I assume the if could just be dropped?

marcoow

great job 🎉

BobrImperator requested changes Oct 5, 2021

View reviewed changes

BobrImperator reviewed Oct 5, 2021

View reviewed changes

BobrImperator added the internal label Oct 6, 2021

candunaj force-pushed the 2316-refactor-mocha-tests-to-qunit branch from 85aafb9 to c595c8c Compare October 8, 2021 21:11

candunaj requested a review from marcoow October 20, 2021 13:36

candunaj force-pushed the 2316-refactor-mocha-tests-to-qunit branch 2 times, most recently from 27c3c4f to 8702ed2 Compare October 26, 2021 08:24

candunaj force-pushed the 2316-refactor-mocha-tests-to-qunit branch from 8702ed2 to 3cd3507 Compare October 29, 2021 11:03

candunaj changed the title ~~2316 refactor mocha tests to qunit~~ [WIP] 2316 refactor mocha tests to qunit Nov 4, 2021

candunaj force-pushed the 2316-refactor-mocha-tests-to-qunit branch from 3cd3507 to 142c23a Compare November 4, 2021 15:11

candunaj requested a review from BobrImperator November 4, 2021 15:14

candunaj changed the title ~~[WIP] 2316 refactor mocha tests to qunit~~ 2316 refactor mocha tests to qunit Nov 12, 2021

marcoow requested changes Nov 12, 2021

View reviewed changes

candunaj force-pushed the 2316-refactor-mocha-tests-to-qunit branch 8 times, most recently from 69a2dd9 to e9d4eb7 Compare November 30, 2021 07:30

candunaj assigned marcoow Nov 30, 2021

candunaj requested a review from marcoow November 30, 2021 07:42

marcoow reviewed Nov 30, 2021

View reviewed changes

candunaj force-pushed the 2316-refactor-mocha-tests-to-qunit branch 2 times, most recently from f78d5d6 to 5b35bbd Compare December 9, 2021 10:53

candunaj force-pushed the 2316-refactor-mocha-tests-to-qunit branch from 5b35bbd to ee701a8 Compare December 9, 2021 11:07

candunaj added 2 commits December 10, 2021 12:21

Mocha replaced with qunit

a194731

Refactored all tests to qunit

57f659e

candunaj force-pushed the 2316-refactor-mocha-tests-to-qunit branch from ee701a8 to 57f659e Compare December 10, 2021 11:22

candunaj requested a review from marcoow December 10, 2021 11:25

marcoow approved these changes Dec 10, 2021

View reviewed changes

BobrImperator approved these changes Dec 10, 2021

View reviewed changes

marcoow merged commit 9fde2c2 into master Dec 10, 2021

delete-merged-branch bot deleted the 2316-refactor-mocha-tests-to-qunit branch December 10, 2021 11:33

marcoow mentioned this pull request Apr 29, 2022

[WIP] Remove ember-cli-mocha dependency #2242

Closed

BobrImperator mentioned this pull request Jul 22, 2022

Upgrade ember-qunit to v5 #2434

Closed

BobrImperator mentioned this pull request Feb 6, 2023

Migrate Mocha to Qunit #2316

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2316 refactor mocha tests to qunit #2330

2316 refactor mocha tests to qunit #2330

candunaj commented Oct 5, 2021 •

edited

Loading

BobrImperator Oct 5, 2021

candunaj Oct 5, 2021

BobrImperator Oct 5, 2021

BobrImperator Oct 5, 2021

BobrImperator Oct 5, 2021

BobrImperator Oct 5, 2021

marcoow commented Oct 20, 2021

candunaj commented Oct 20, 2021 via email

marcoow commented Oct 20, 2021

marcoow commented Oct 29, 2021

marcoow left a comment

marcoow Nov 12, 2021

candunaj Nov 15, 2021

marcoow Nov 15, 2021

candunaj Nov 15, 2021

marcoow Nov 12, 2021

marcoow Nov 12, 2021

marcoow Nov 12, 2021

marcoow Nov 12, 2021

candunaj Nov 15, 2021

marcoow Nov 15, 2021

candunaj Nov 26, 2021 •

edited

Loading

marcoow Nov 26, 2021

marcoow Nov 12, 2021

marcoow Nov 12, 2021

marcoow Nov 12, 2021

marcoow Nov 12, 2021

BobrImperator commented Nov 25, 2021 •

edited

Loading

marcoow Nov 30, 2021

marcoow Nov 30, 2021

marcoow left a comment

		@@ -1,4 +1,4 @@
		import { alias, oneWay } from '@ember/object/computed';
		import { alias, readOnly } from '@ember/object/computed';

2316 refactor mocha tests to qunit #2330

2316 refactor mocha tests to qunit #2330

Conversation

candunaj commented Oct 5, 2021 • edited Loading

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

marcoow commented Oct 20, 2021

candunaj commented Oct 20, 2021 via email

marcoow commented Oct 20, 2021

marcoow commented Oct 29, 2021

marcoow left a 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

Choose a reason for hiding this comment

candunaj Nov 26, 2021 • edited Loading

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

BobrImperator commented Nov 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoow left a comment

Choose a reason for hiding this comment

candunaj commented Oct 5, 2021 •

edited

Loading

candunaj Nov 26, 2021 •

edited

Loading

BobrImperator commented Nov 25, 2021 •

edited

Loading