Skip to content

[Debugger] Fix log snapshot tests #5600

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+10 −3
Open

[Debugger] Fix log snapshot tests #5600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions tests/debugger/test_debugger_probe_snapshot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import tests.debugger.utils as debugger


from utils import scenarios, features, missing_feature, context, irrelevant, bug
from utils import scenarios, features, missing_feature, context, irrelevant
from utils.interfaces._library.miscs import validate_process_tags


Expand Down Expand Up @@ -213,7 +213,6 @@ class Test_Debugger_Line_Probe_Snaphots(BaseDebuggerProbeSnaphotTest):
def setup_log_line_snapshot(self):
self._setup("probe_snapshot_log_line", "/debugger/log", "log", lines=None)

@bug(context.library == "nodejs", reason="DEBUG-4611")
def test_log_line_snapshot(self):
self._assert()
self._validate_snapshots()
Expand Down Expand Up @@ -298,7 +297,6 @@ class Test_Debugger_Line_Probe_Snaphots_With_SCM(BaseDebuggerProbeSnaphotTest):
def setup_log_line_snapshot(self):
self._setup("probe_snapshot_log_line", "/debugger/log", "log", lines=None)

@bug(context.library == "nodejs", reason="DEBUG-4611")
def test_log_line_snapshot(self):
self._assert()
self._validate_snapshots()
Expand Down
3 changes: 3 additions & 0 deletions tests/debugger/utils/probes/probe_snapshot_log_line.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
"id": "",
"version": 0,
"captureSnapshot": true,
"capture": {
Copy link
Member

@jpbempel jpbempel Oct 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

capture object is optional and should not be a requirement for a probe definition

Copy link
Contributor Author

@watson watson Oct 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

As I wrote in the PR description, that test case should preferably be a separate system test. It hasn't been something that has been an issue since we launched the Node.js tracer, so I would say it's only optional in theory.

But I agree it would be good to support that, just not for this test, whose purpose is different.

Copy link
Member

Choose a reason for hiding this comment

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

Capture object has well defined default values from the beginning, so to me it does not make sense to put this artifical capture object here

Copy link
Contributor Author

@watson watson Oct 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For context, captureSnapshot: true was added to this test this week. This broke CI for the Node.js tracer, because the PR that landed it skipped the Node.js tests before landing. Then shortly after, the @bug line was added for Node.js to make Node.js CI green again. However, I think it's better, for now, to actually run the test for Node.js instead of just labeling it as a bug (which in the real world, it's not)

Copy link
Member

Choose a reason for hiding this comment

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

captureSnapshot is required because it makes the difference between a regular log probe and a snapshot probe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding the @bug doesn't run this test and hence hides any issues related to what this test is testing. Also, it's incorrect calling it a bug as our backend is always adding the capture object to the probe payload sent via RC. I agree that we should probably support the case where it's not there if we ever decide to remove it in some scenarios, but since we haven't done that yet, it's not a bug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we add a test for this scenario, we can add a @bug or @missing_feature for Node.js to that specifically, while still allowing other tests to run that tests other things.

Copy link
Member

Choose a reason for hiding this comment

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

By default the UI happens to send :

  "capture": {
    "maxReferenceDepth": 3
  },

but at first (as the option was not exposed) it was sending nothing.
Also the IDE plugin can also choose to send nothing.
You are not guarantee to always have this cpature object.

Copy link
Contributor Author

@watson watson Oct 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not saying that we shouldn't support capture not being there in the tracers. I'll make a system test to verify this (WIP PR here) 👍 What I meant was, that what this test is currently failing on for Node.js, is not what the test is designed to test, and I want to unblock it, so we can get the test coverage we need in Node.js.

We have so far said that our system tests are the source of truth, because we don't have a spec defining the probe structure. I've never liked this, exactly due to this ambiguity. But since no system test in the past have sent a probe without also including the capture property, the Node.js tracer hasn't marked it as optional (remember this test landed by a mistake because it didn't correctly run the Node.js tests before being merged).

I'm 100% on board to fix this, but I believe it's outside the scope of this PR, and unless there's a good reason to test this specifically in this PR, I'd prefer to merge it ASAP - hope you understand 😀

Copy link
Member

Choose a reason for hiding this comment

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

If the test fails only for Node.js you have 2 options to fix that:

  • introduce a fake capture like you did
  • mark the test with @bug for current and past version. Once it is fixed with the new release, the test will be performed for the next releases. Yes the test will not be performed for the current release. is it a big deal I don't think so. It is passing currently with a capture. You will not change past releases anyway

"maxFieldCount": 200
},
"where": {
"typeName": null,
"sourceFile": "ACTUAL_SOURCE_FILE",
Expand Down
6 changes: 6 additions & 0 deletions tests/debugger/utils/probes/probe_snapshot_log_mixed.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
"id": "",
"version": 0,
"captureSnapshot": true,
"capture": {
"maxFieldCount": 200
},
"where": {
"typeName": "ACTUAL_TYPE_NAME",
"methodName": "MixProbe",
Expand All @@ -17,6 +20,9 @@
"id": "",
"version": 0,
"captureSnapshot": true,
"capture": {
"maxFieldCount": 200
},
"where": {
"typeName": null,
"sourceFile": "ACTUAL_SOURCE_FILE",
Expand Down
Loading