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

Add InspectableTimestampedPointContext to TestEventDetails #9813

Merged
merged 3 commits into from Oct 5, 2023

Conversation

hbenl
Copy link
Collaborator

@hbenl hbenl commented Oct 5, 2023

The objects shown in the TestEventDetails panel are from a different point than the one that we seek to when the user clicks on the test step. If the objects contain an HTML element and the user clicks the inspect button on that element (to show it in the Elements panel), we need to seek to that point. This is achieved by adding an InspectableTimestampedPointContext containing that point to TestEventDetails.
This fixes one of the issues in FE-1962, see this comment.

@hbenl hbenl self-assigned this Oct 5, 2023
@vercel
Copy link

vercel bot commented Oct 5, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
devtools ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 5, 2023 2:33pm

@replay-io
Copy link

replay-io bot commented Oct 5, 2023

E2E Tests

82 replays were recorded for 7b8a5a1.

image 2 Failed
  • authenticated/test-suites/library-test-runs
      expect(received).toBe(expected) // Object.is equality
      Expected: 1
      Received: 0
  • authenticated/test-suites/library-tests-list
      locator.click: Timeout 15000ms exceeded.
      =========================== logs ===========================
      waiting for locator('[data-test-id="TestRunListItem"]').filter({ has: locator('[data-test-id="TestRun-Title"]').filter({ hasText: 'github: remove semgrep (#9332)' }) }).first()
      ============================================================
image 80 Passed

View test run on Replay ↗︎

Snapshot Tests

99 replays were recorded for 7b8a5a1.

image 0 Failed
image 99 Passed

View test run on Replay ↗︎

Copy link
Collaborator

@markerikson markerikson left a comment

Choose a reason for hiding this comment

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

Approved in general, I'm just not sure why the useMemo is even needed :)

@@ -62,6 +63,14 @@ function UserActionEventDetails({
variable
);

const context = useMemo(
Copy link
Collaborator

Choose a reason for hiding this comment

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

❓ is there even any reason to memoize this? Why not just pass value={timeStampedPoint} directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately this is necessary because TimeStampedPoint has a point property but InspectableTimestampedPointContext expects an executionPoint property.

@hbenl hbenl merged commit bf2a348 into main Oct 5, 2023
34 of 36 checks passed
@hbenl hbenl deleted the hbenl/add-timestampedpointcontext branch October 5, 2023 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants