Skip to content

[lldb] Fix TestModuleLoadedNotifys API test to work correctly on most of Linux targets #94672

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

Merged
merged 4 commits into from
Jun 10, 2024
Merged

[lldb] Fix TestModuleLoadedNotifys API test to work correctly on most of Linux targets #94672

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3f91eca
[lldb] Fix TestModuleLoadedNotifys API test to work correctly on most…
slydiman Jun 6, 2024
0518bff
Keep the `avg_solibs_added_per_event` name correct.
slydiman Jun 7, 2024
60671ec
Added own so modules.
slydiman Jun 8, 2024
256a245
Optimized.
slydiman Jun 10, 2024
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: 2 additions & 2 deletions lldb/test/API/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,6 @@ def test_launch_notifications(self):
# On Linux we get events for ld.so, [vdso], the binary and then all libraries.

avg_solibs_added_per_event = round(
float(total_solibs_added) / float(total_modules_added_events)
10.0 * float(total_solibs_added) / float(total_modules_added_events)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can't really call it "number of libraries per event" anymore if you multiply by ten. It's more like number of decilibraries/event :P. Maybe you could just remove the round call, and compare to 1.0, but I'm wondering if this is even the right metric for the test. Since what we're trying to check is that we don't send these notifications one by one, I think some check like "number of events with more than one module" or "number of total events" (or both) would be better. @jasonmolenda, what do you think?

Also, this test is very unhermetic in the sense that it depends on the environment to provide enough shared libraries to measure. In the extreme case we could have a totally statically linked binary and zero shared libraries. So, another way to make this test be more resilient is to introduce a couple of shared libraries of our own, so that we can be sure there is something to measure. You could copy the pattern from TestLoadUnload.py to link a bunch of shared libraries to this binary.

Copy link
Contributor Author

@slydiman slydiman Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You can't really call it "number of libraries per event" anymore if you multiply by ten.

I have updated the patch.

Maybe you could just remove the round call, and compare to 1.0

No, because assertGreater() expects int parameters.

Also, this test is very unhermetic

Right, and it seems that the behavior on Darwin is very different. I'd just check min_modules_per_event >= 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@labath

So, another way to make this test be more resilient is to introduce a couple of shared libraries of our own

The patch is updated this way. Please review. Thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can't really call it "number of libraries per event" anymore if you multiply by ten.

I have updated the patch.

Maybe you could just remove the round call, and compare to 1.0

No, because assertGreater() expects int parameters.

That's strange because assertGreater(47.0001, 47.0000) works just fine for me. So does assertLessEqual(set(["a", "b"]), set(["a", "b"])).

Could it be that you're still running on the version of lldb that has the ancient checked in copy of unittest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just checked the documentation for assertGreater() trying to figure out the purpose of round() usage in the original code. But it does not matter anymore, since the patch has been significantly refactored.

)
self.assertGreater(avg_solibs_added_per_event, 1)
self.assertGreater(avg_solibs_added_per_event, 10)
Loading