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

Fix/removed snapshot tests in operation details table #2674

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

Conversation

Mubashirshariq
Copy link

Which problem is this PR solving?

Resolves #2659

Description of the changes

  • Uses RTL's queries that reflect how users interact with the UI
  • Tests actual component behavior rather than implementation details
  • Verifies accessibility by using role-based queries
  • Tests formatting logic explicitly rather than through snapshots
  • Covers key user interactions like sorting and row hovering
  • Includes proper cleanup of mocks between tests
  • Better error messages when tests fail (compared to snapshot diffs)

How was this change tested?

Checklist

Signed-off-by: Mubashirshariq <[email protected]>
Signed-off-by: Mubashirshariq <[email protected]>
@Mubashirshariq Mubashirshariq requested a review from a team as a code owner February 19, 2025 19:18
@Mubashirshariq Mubashirshariq requested review from mahadzaryab1 and removed request for a team February 19, 2025 19:18
// See the License for the specific language governing permissions and
// limitations under the License.

import '@testing-library/jest-dom';
Copy link
Member

Choose a reason for hiding this comment

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

why do we need this?

Copy link
Author

Choose a reason for hiding this comment

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

Tobeindocument doesn't work unless we add setupTests.js.
refer this

Copy link
Member

Choose a reason for hiding this comment

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

we already have tests that use this function without any new dependencies or setup step

packages/jaeger-ui/src/components/common/ErrorMessage.test.js
51:    expect(screen.getByTestId('ErrorMessage--details--wrapper')).toBeInTheDocument();

package.json Outdated
"dependencies": {
"@testing-library/jest-dom": "^6.6.3",
"@testing-library/react": "^16.2.0",
"@testing-library/user-event": "^14.6.1"
Copy link
Member

Choose a reason for hiding this comment

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

these are not runtime dependencies

Copy link
Author

Choose a reason for hiding this comment

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

Earlier i added them as dev dependencies but the n npm run lint started to complain to add it to dependencies rather than dev dependency

Copy link
Author

Choose a reason for hiding this comment

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

will run lint again,will try to add them to devdependencies

Signed-off-by: Mubashirshariq <[email protected]>
@Mubashirshariq
Copy link
Author

@yurishkuro i have moved RTL to dev dependencies but the issue i am getting running while npm run lint is same ,attaching the ss for the same
Screenshot 2025-02-21 at 10 36 16 AM

@hari45678
Copy link
Contributor

@Mubashirshariq You are trying to install the dependencies in project's root. They need to be in the respective workspace, i.e., jaeger-ui.
Use command npm install abc -w packages/jaeger-ui instead of the normal npm install abc

@Mubashirshariq
Copy link
Author

@Mubashirshariq You are trying to install the dependencies in project's root. They need to be in the respective workspace, i.e., jaeger-ui. Use command npm install abc -w packages/jaeger-ui instead of the normal npm install abc

@hari45678 yeah i also figured it out now only

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.

Avoid snapshot in operationDetailsTable/index.test.js
3 participants