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

test: delete pummel/test-dtrace-jsstack #26869

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 22, 2019

test: delete pummel/test-dtrace-jsstack

The test pummel/test-dtrace-jsstack is broken and probably has been for
a very long time. Remove it.

It gets skipped on anything that is non-SunOS. In our CI, that means
skipped everywhere but SmartOS.

When run on SmartOS in our CI (which never happens because it's in
pummel, but I moved it into sequential to test it), it fails because it
needs elevated privileges.

When I log into the SmartOS machine and run the test as root, it fails
with:

  AssertionError [ERR_ASSERTION]: did not find expected frame stalloogle

Since I have dtrace installed on my macOS machine, I tried running it
locally but removing the SunOS check. It failed because the test leaks a
global variable. I removed the global leak check, and the test failed
because I have System Integrity Protection enabled.

In short, the test does not work in its current form, has almost
certainly not worked in a long time, and is very likely to be brittle if
we ever do fix it. I'm inclined to remove it.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 22, 2019
@Trott Trott changed the title test: move test-dtrace-jsstack out of pummel test: delete pummel/test-dtrace-jsstack Mar 22, 2019
The test pummel/test-dtrace-jsstack is broken and probably has been for
a very long time. Remove it.

It gets skipped on anything that is non-SunOS. In our CI, that means
skipped everywhere but SmartOS.

When run on SmartOS in our CI (which never happens because it's in
pummel, but I moved it into sequential to test it), it fails because it
needs elevated privileges.

When I log into the SmartOS machine and run the test as root, it fails
with:

  AssertionError [ERR_ASSERTION]: did not find expected frame stalloogle

Since I have dtrace installed on my macOS machine, I tried running it
locally but removing the SunOS check. It failed because the test leaks a
global variable. I removed the global leak check, and the test failed
because I have System Integrity Protection enabled.

In short, the test does not work in its current form, has almost
certainly not worked in a long time, and is very likely to be brittle if
we ever do fix it. I'm inclined to remove it.
@Trott Trott marked this pull request as ready for review March 22, 2019 21:56
@nodejs nodejs deleted a comment from nodejs-github-bot Mar 22, 2019
@Trott
Copy link
Member Author

Trott commented Mar 22, 2019

@nodejs/platform-smartos @nodejs/post-mortem

@Trott
Copy link
Member Author

Trott commented Mar 22, 2019

@misterdjules
Copy link

This test is about testing the DTrace ustack helper functionality for Node.js so that users can profile Node.js programs using DTrace and get function names for JS frames. It seems that the target tier for that functionality is 3 according to the diagnostics support tiers table.

Description of Tier 3 is:

If possible its test suite will be run at least nightly in the Node.js CI and issues opened for failures. Does not block shipping a release.

So it seems at some point the goal was for some folks to have this functionality work. I don't have time to spend to fix that so I'll defer to others to determine what to do there, but if that test is removed the target tier for this functionality should probably be downgraded and that should be socialized with the broader community of potential users.

@joyeecheung
Copy link
Member

The description is

If possible its test suite will be run at least nightly in the Node.js CI and issues opened for failures

With the current permission setup of our CI, I guess it is not possible to properly run this. I am also inclined to remove it, until someone figure out how to work around the privilege issue in our infra.

@Trott
Copy link
Member Author

Trott commented Mar 23, 2019

if that test is removed the target tier for this functionality should probably be downgraded

One thing that maybe I should clarify: The test isn't run on SmartOS in CI because, until recently, none of the tests in pummel ran on CI at all ever. We've recently started running pummel tests once a day on a single platform as a sort of canary. But the canary won't work for platform-specific tests. So I tried moving this one to sequential but it became clear that it's been broken for a long time.

So the test isn't being removed because of it's target. It's being removed because it's broken, been broken for a long time, and nobody seems to have noticed or cared. So one possible conclusion (although admittedly not necessarily the correct conclusion, so additional info welcome) is that whatever functionality it's testing (which seems to lie at least partially outside of Node.js) is perhaps not actually something we need to test. Like, if this test fails, is Node.js broken? Or is it (for example) perhaps merely incompatible with dtrace and that actually might be OK?

My dtrace knowledge and post-mortem knowledge are pretty dismal so another possibility is that there is a trivial fix to the test. I'm drawing the best conclusion I can here based on what I know, and am hoping others will either confirm that I'm basically right or else do better and get this going in the right direction.

@Trott
Copy link
Member Author

Trott commented Mar 23, 2019

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 23, 2019
@joyeecheung
Copy link
Member

So one possible conclusion (although admittedly not necessarily the correct conclusion, so additional info welcome) is that whatever functionality it's testing (which seems to lie at least partially outside of Node.js) is perhaps not actually something we need to test. Like, if this test fails, is Node.js broken? Or is it (for example) perhaps merely incompatible with dtrace and that actually might be OK?

We could also try parsing the errors from dtrace and skip if it's permission issue, but with our CI setup if it's going to be skipped always, then it's still not really useful to keep it, at least not in a folder that is run by the CI every day.

@Trott
Copy link
Member Author

Trott commented Mar 24, 2019

Landed in 4ba33c9

@Trott Trott closed this Mar 24, 2019
Trott added a commit to Trott/io.js that referenced this pull request Mar 24, 2019
The test pummel/test-dtrace-jsstack is broken and probably has been for
a very long time. Remove it.

It gets skipped on anything that is non-SunOS. In our CI, that means
skipped everywhere but SmartOS.

When run on SmartOS in our CI (which never happens because it's in
pummel, but I moved it into sequential to test it), it fails because it
needs elevated privileges.

When I log into the SmartOS machine and run the test as root, it fails
with:

  AssertionError [ERR_ASSERTION]: did not find expected frame stalloogle

Since I have dtrace installed on my macOS machine, I tried running it
locally but removing the SunOS check. It failed because the test leaks a
global variable. I removed the global leak check, and the test failed
because I have System Integrity Protection enabled.

In short, the test does not work in its current form, has almost
certainly not worked in a long time, and is very likely to be brittle if
we ever do fix it. I'm inclined to remove it.

PR-URL: nodejs#26869
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@misterdjules
Copy link

@Trott @joyeecheung

I didn't want to suggest that removing the test was not a good action to take.

I'm suggesting that if that test fails, and given my previous experience and knowledge with using the DTrace ustack helper on SmartOS, it is likely that this functionality is broken and would require a significant amount of work to be fixed.

So if folks are even less likely to notice that this functionality is broken because the test is removed, it seems having the goal for that functionality to be at the third level of support tier may be unrealistic, and that maybe be worth communicating with the @nodejs/diagnostics group.

targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
The test pummel/test-dtrace-jsstack is broken and probably has been for
a very long time. Remove it.

It gets skipped on anything that is non-SunOS. In our CI, that means
skipped everywhere but SmartOS.

When run on SmartOS in our CI (which never happens because it's in
pummel, but I moved it into sequential to test it), it fails because it
needs elevated privileges.

When I log into the SmartOS machine and run the test as root, it fails
with:

  AssertionError [ERR_ASSERTION]: did not find expected frame stalloogle

Since I have dtrace installed on my macOS machine, I tried running it
locally but removing the SunOS check. It failed because the test leaks a
global variable. I removed the global leak check, and the test failed
because I have System Integrity Protection enabled.

In short, the test does not work in its current form, has almost
certainly not worked in a long time, and is very likely to be brittle if
we ever do fix it. I'm inclined to remove it.

PR-URL: nodejs#26869
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Mar 27, 2019
The test pummel/test-dtrace-jsstack is broken and probably has been for
a very long time. Remove it.

It gets skipped on anything that is non-SunOS. In our CI, that means
skipped everywhere but SmartOS.

When run on SmartOS in our CI (which never happens because it's in
pummel, but I moved it into sequential to test it), it fails because it
needs elevated privileges.

When I log into the SmartOS machine and run the test as root, it fails
with:

  AssertionError [ERR_ASSERTION]: did not find expected frame stalloogle

Since I have dtrace installed on my macOS machine, I tried running it
locally but removing the SunOS check. It failed because the test leaks a
global variable. I removed the global leak check, and the test failed
because I have System Integrity Protection enabled.

In short, the test does not work in its current form, has almost
certainly not worked in a long time, and is very likely to be brittle if
we ever do fix it. I'm inclined to remove it.

PR-URL: #26869
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Trott Trott deleted the sunos-only branch January 13, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants