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

Enhance react integration test #2389

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

gxueatlassian
Copy link
Contributor

What's in this PR?
Enhance the react integration test.

Why

  1. Instead of mocking the API, use lib to mock backend (http) directly. So that we are testing the api client as well.
  2. Usecase base testing, give more authentic testing.

Added feature flags

Affected issues
N/A

How has this been tested?
Integration Test

Whats Next?

@gxueatlassian gxueatlassian requested a review from a team as a code owner September 1, 2023 06:16
krazziekay
krazziekay previously approved these changes Sep 1, 2023
@gxueatlassian gxueatlassian enabled auto-merge (squash) September 4, 2023 02:49

describe("Empty state user flow", () => {

let mockWinOpenFunc: jest.Mock;
Copy link
Contributor

Choose a reason for hiding this comment

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

/nit As someone looking at this with no context, the function name isn't super clear. Does it mean mockWindowOpenFunc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have abstracted it away

}

async function clickButton(buttonText: string) {
await act(async() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to move these to a different file to allow them to be reused more globally or moved directly into the test case if they won't be reused at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup have moved it in a utils file.

}

function expectUserLoginSuccess(login: string) {
expect(screen.queryByText("Select your GitHub product")).not.toBeInTheDocument();
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a personal preference for not adding assertions in a function, because it may be confusing to someone writing test cases if a "hidden" assertion fails. Would love to hear more about your opinion on this @gxueatlassian

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My idea is the treat expectUserLoginSuccess as any other expectXXX.
If a "hidden" assertion fail, dev shouldn't be confused that something fail inside expectUserLoginSuccess, although dev still need to look inside to see what's failing.

I am keen to keep this as it is, but I can change it if you strongly suggest should use plain expect.I am also okay with that.

@@ -0,0 +1 @@
module.exports = {};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In some react component we use import xxx.css.
Seems the OrgsContainer we import ScrollBar which import SimpleBar which import xx.css and ts-jest is failing on recongise it.
Search web give me answer that to put a stub here just to pase the ts check.

@@ -3,6 +3,9 @@
"transform": {
"^.+\\.tsx?$": "ts-jest"
},
"moduleNameMapper": {
"^.+\\.css$": "<rootDir>/css.stub.js"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

@@ -1,4 +1,3 @@
import "@testing-library/jest-dom";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To speed up the test coz we don't need it in each file, just the setup.ts file.

@@ -22,5 +23,5 @@
"rest-interfaces": ["./src/rest-interfaces"]
}
},
"include": ["src"]
"includes": [ "./setup.ts", "src" ]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise doesn't looks like the ts-jest is including the tesing-lib/js-dom thing during the ts compile phase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean after I remove import "@testing-library/jest-dom"; tests failed, turns out that import "@testing-library/jest-dom"; is not called by ts-jest during compile.

ps: is has nothing to do with jest at compile time.

@gxueatlassian gxueatlassian marked this pull request as draft November 6, 2023 00:40
auto-merge was automatically disabled November 6, 2023 00:40

Pull request was converted to draft

@gxueatlassian gxueatlassian marked this pull request as ready for review November 6, 2023 01:21
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.

3 participants