Skip to content
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.

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

[no-issue]: Convert web-config-server tests to jest #4194

Merged
merged 10 commits into from
Oct 3, 2022

Conversation

kael89
Copy link
Collaborator

@kael89 kael89 commented Sep 28, 2022

1 package left (central-server) and then mocha can go! 🎉

Most of the job was done using a community codemod.

A few other tweaks too, see commits for an overview

@@ -1,6 +1,6 @@
{
"clearMocks": true,
"collectCoverageFrom": ["<rootDir>/src/**/*.js)"],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems that nobody was checking test coverage in JS packages for the last 14 months 😛

@@ -1,10 +1,3 @@
const getPlugins = api => {
if (api.env(['development', 'test'])) {
return ['istanbul'];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Build-related change, should be fine

...baseConfig,
rootDir: '.',
moduleNameMapper: {
'^/aggregator(.*)$': '<rootDir>/src/aggregator$1',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't find a way to generalise these (without more elaborate folder scanning logic).

I also tried the following transformation. It throws an error because it is also applied to node_modules packages:

'^/(.*)$': '<rootDir>/src/$1'

packages/web-config-server/jest.setup.js Outdated Show resolved Hide resolved
},
};

describe.skip('UserHasAccess', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These tests actually fail and have been disabled for as long as I remember them... I'd suggest to just delete them

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm happy to delete them, what about you @edmofro

let app;

beforeAll(async () => {
app = new TestableApp(await createApp());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that createApp() needs not be async: app.js

It is not async in central-server

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, doesn't need async, can you remove it @kael89? 🙏

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Happy to do so in another PR though! (Don't want touch production code in this one)

Copy link
Collaborator

@rohan-bes rohan-bes left a comment

Choose a reason for hiding this comment

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

Oh wow! This is so awesome!! Thanks @kael89 😄 The open-source community is becoming a real powerhouse!

Just a couple small comments, but pre-approving.

entityHierarchy: { findById: findEntityHierarchyById },
};

describe('ReportServerDataBuilder', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are new tests? Thanks so much for adding them! 🙌

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No just a rename and a bit of refactoring. Git thinks they are new though 🙂

},
};

describe.skip('UserHasAccess', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm happy to delete them, what about you @edmofro

let app;

beforeAll(async () => {
app = new TestableApp(await createApp());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, doesn't need async, can you remove it @kael89? 🙏

@kael89 kael89 force-pushed the convert-web-config-server-tests-to-jest branch from 53e4b74 to dae0003 Compare October 3, 2022 22:59
@kael89 kael89 merged commit a34a3b7 into dev Oct 3, 2022
@kael89 kael89 deleted the convert-web-config-server-tests-to-jest branch October 3, 2022 23:52
@rohan-bes rohan-bes mentioned this pull request Oct 10, 2022
4 tasks
@biaoli0 biaoli0 mentioned this pull request Oct 17, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants