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

4832 / Skipped test #2210

Merged
merged 17 commits into from
Jun 21, 2023
Merged

4832 / Skipped test #2210

merged 17 commits into from
Jun 21, 2023

Conversation

snony14
Copy link
Contributor

@snony14 snony14 commented Jun 6, 2023

No description provided.

@github-actions
Copy link

github-actions bot commented Jun 6, 2023

(auto-deploy) A deployment has been created for this Pull Request

Preview links

As part of the code review process, please ensure that you test against the following

Version URL
Web https://web.env.reactivetrader.com/pull/2210
OpenFin - FX fins://openfin.env.reactivetrader.com/pull/2210/config/rt-fx.json
OpenFin - Credit fins://openfin.env.reactivetrader.com/pull/2210/config/rt-credit.json
OpenFin - Launcher fins://openfin.env.reactivetrader.com/pull/2210/config/launcher.json
Finsemble https://finsemble.env.reactivetrader.com/pull/2210

Performance

Please ensure that this PR does not degrade the performance of the UI. We should maintain a performance score of 95+.

https://developers.google.com/speed/pagespeed/insights/?url=https://web.env.reactivetrader.com/pull/2210

@snony14 snony14 self-assigned this Jun 8, 2023
Copy link
Contributor Author

@snony14 snony14 Jun 8, 2023

Choose a reason for hiding this comment

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

I deleted this test because, under the hood, it is testing the rendering of SVG which creates a Dompoint . The current testing environment is run under jsdom, and SVG is not deeply tested, so calling something like this svg.createSVGPoint() is throwing a type error; please see link. createSVGPoint() is called in the file Tooltip.tsx, line 119.

act(() => {
userEvent.selectOptions(
Copy link
Contributor Author

@snony14 snony14 Jun 9, 2023

Choose a reason for hiding this comment

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

This way of selecting an option for a selector was not working, which subsequentially led the test to fail.

@@ -69,6 +69,7 @@ export const NumFilter = ({
<FilterPopup
parentRef={parentRef}
ariaLabel={`Filter trades by ${colDef[field].headerName} field value`}
leftAlignFilter={field !== "tradeId"}
Copy link
Contributor Author

@snony14 snony14 Jun 9, 2023

Choose a reason for hiding this comment

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

The alignment for numeric field in the blotter were not displayed in the screen. Please see ReactiveTrader and click the Filter Icon for Notional, or Rate. This is now fixed, see image below:

numeric_dialog

@@ -119,33 +119,4 @@ describe("Positions", () => {

expect(screen.getByTestId("tooltip").textContent).toBe("AUD -1,557,031")
})

// TODO (5350): fix test failure - Windows snapshot incompatible with Mac
it.skip("should display the correct bubble chart", async () => {
Copy link
Contributor Author

@snony14 snony14 Jun 9, 2023

Choose a reason for hiding this comment

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

Since the aim was to generate a snapshot, I have removed it as @algreasley suggested that the test should focus on component testing for user events such as clicking a button, etc...

@snony14 snony14 marked this pull request as ready for review June 14, 2023 12:09
@snony14 snony14 removed their assignment Jun 20, 2023
@snony14 snony14 merged commit 7b3c00b into master Jun 21, 2023
5 checks passed
@snony14 snony14 deleted the 4832/skipped-test branch June 21, 2023 15:23
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