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

Migrate TraceTimelineLink.test.js from enzyme to RTL #1960

Merged
merged 2 commits into from
Nov 10, 2023
Merged

Migrate TraceTimelineLink.test.js from enzyme to RTL #1960

merged 2 commits into from
Nov 10, 2023

Conversation

EshaanAgg
Copy link
Contributor

Which problem is this PR solving?

Fixes part of #1668

Description of the changes

Migrated the TraceTimelineLink.test.js to RTL using the idiomatic RTL practices

How was this change tested?

Ran the test suite to ensure all changes passed.

Checklist

@EshaanAgg EshaanAgg requested a review from a team as a code owner November 9, 2023 18:50
@EshaanAgg EshaanAgg requested review from yurishkuro and removed request for a team November 9, 2023 18:50
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Thanks!

});

it('links to the given trace', () => {
expect(wrapper.find('a').prop('href')).toBe(`/trace/${traceID}`);
expect(screen.getByRole('link').href).toBe(`http://localhost/trace/${traceID}`);
Copy link
Member

Choose a reason for hiding this comment

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

interesting, why is this changing from no-host URL to http://localhost? That seems counterintuitive - localhost is very dependent on where the code runs

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because AFAIK while rendering RTL actually converts all the the relative links to the actual complete URLs to which the same anchor is supposed to route to. I get the feeling of it being a bit counterintuitive, but since the tests would always be running in either locally or in workflows (and never in build or production), the same shouldn't be ambiguous or flaky.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't see any other tests asserting full URL with host name (unless the test provided the value like example.com), so I added URL parsing bit

Signed-off-by: Yuri Shkuro <[email protected]>
@yurishkuro yurishkuro added the changelog:test Change that's adding missing tests or correcting existing tests label Nov 10, 2023
@yurishkuro yurishkuro changed the title refac: Migrate TraceTimelineLink.test.js from enzyme to RTL Migrate TraceTimelineLink.test.js from enzyme to RTL Nov 10, 2023
@yurishkuro yurishkuro enabled auto-merge (squash) November 10, 2023 02:14
});

it('links to the given trace', () => {
expect(wrapper.find('a').prop('href')).toBe(`/trace/${traceID}`);
expect(screen.getByRole('link').href).toBe(`http://localhost/trace/${traceID}`);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expect(screen.getByRole('link').href).toBe(`http://localhost/trace/${traceID}`);
expect(screen.getByRole('link')).toHaveAttribute('href', `/trace/${traceID}`);

I think checking for the jsx dom attribute would make more sense in this case rather than the rendered dom attribute.

Copy link
Member

Choose a reason for hiding this comment

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

expect(...).toHaveAttribute is not a function

but changing to toHaveProperty still results in full URL:

    expect(received).toHaveProperty(path, value)

    Expected path: "href"

    Expected value: "/trace/test-trace-id"
    Received value: "http://localhost/trace/test-trace-id"

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you need to import import '@testing-library/jest-dom'; for it to work

@@ -13,30 +13,31 @@
// limitations under the License.

import * as React from 'react';
import { shallow } from 'enzyme';
import { render, screen, fireEvent, createEvent } from '@testing-library/react';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { render, screen, fireEvent, createEvent } from '@testing-library/react';
import { render, screen, fireEvent, createEvent } from '@testing-library/react';
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.

Why do we import the jest-dom environment here?

Copy link
Member

@anshgoyalevil anshgoyalevil Nov 10, 2023

Choose a reason for hiding this comment

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

Functions like toHaveAttribute can be used with that import, but since Yuri has added a new URL parsing logic, this change is not required.

Copy link
Member

Choose a reason for hiding this comment

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

ah, with this ^ the other suggestion works. But I prefer a more explicit / brute-force new URL().pathname.

Copy link

codecov bot commented Nov 10, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Files Coverage Δ
.../jaeger-ui/src/components/common/NewWindowIcon.tsx 100.00% <100.00%> (ø)

📢 Thoughts on this report? Let us know!

@yurishkuro yurishkuro merged commit 8902ff4 into jaegertracing:main Nov 10, 2023
10 of 11 checks passed
@yurishkuro
Copy link
Member

@EshaanAgg thank you! 🎈🎈🎈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:test Change that's adding missing tests or correcting existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants