Write a gtest for basic features of the Gecko Profiler

RESOLVED FIXED in Firefox 55

Status

()

Core
Gecko Profiler
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 months ago
The Gecko Profiler's test coverage is pretty bad. Bug 1348776 is a good example of something that should have been discovered via automated testing.

A gtest can test most or all of the profiler's public functions, i.e. profiler_*().
(Assignee)

Comment 1

3 months ago
Created attachment 8853234 [details] [diff] [review]
Write a gtest for basic features of the Gecko Profiler

This required a tweak to DoNativeBacktrace() to work around an ASAN false
positive.
Attachment #8853234 - Flags: review?(mstange)
(Assignee)

Updated

3 months ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Comment on attachment 8853234 [details] [diff] [review]
Write a gtest for basic features of the Gecko Profiler

Review of attachment 8853234 [details] [diff] [review]:
-----------------------------------------------------------------

This is great.

Please remember to remove the calls to the (now deleted) function profiler_is_active_and_not_in_privacy_mode() before landing this.

::: tools/profiler/tests/gtest/GeckoProfiler.cpp
@@ +376,5 @@
> +    profiler_start(PROFILE_DEFAULT_ENTRIES, PROFILE_DEFAULT_INTERVAL,
> +                   features, MOZ_ARRAY_LENGTH(features),
> +                   filters, MOZ_ARRAY_LENGTH(filters));
> +
> +    UniqueProfilerBacktrace bt = profiler_get_backtrace();

You're not doing anything with bt. Is this an oversight, or are you just testing that this doesn't crash?

@@ +385,5 @@
> +  SamplerStackFrameDynamicRAII raii2("A", js::ProfileEntry::Category::STORAGE,
> +                                     888, dynamic.get());
> +  void* handle = profiler_call_enter("A", js::ProfileEntry::Category::NETWORK,
> +                                     this, false, 999);
> +  UniqueProfilerBacktrace bt = profiler_get_backtrace();

Same here.

@@ +386,5 @@
> +                                     888, dynamic.get());
> +  void* handle = profiler_call_enter("A", js::ProfileEntry::Category::NETWORK,
> +                                     this, false, 999);
> +  UniqueProfilerBacktrace bt = profiler_get_backtrace();
> +  profiler_call_exit(handle);

I'd prefer to call these stack classes and functions "implementation details" and not consider them part of the API. Not sure why you're testing them separately after testing the PROFILER_LABEL* macro versions.
Attachment #8853234 - Flags: review?(mstange) → review+
(Assignee)

Comment 3

3 months ago
> Please remember to remove the calls to the (now deleted) function
> profiler_is_active_and_not_in_privacy_mode() before landing this.

Done.

> You're not doing anything with bt. Is this an oversight, or are you just
> testing that this doesn't crash?

Basically just testing that it doesn't crash, but I changed both occurrences to ASSERT_TRUE(profiler_get_backtrace()) to make them consistent with other calls.

> I'd prefer to call these stack classes and functions "implementation
> details" and not consider them part of the API. Not sure why you're testing
> them separately after testing the PROFILER_LABEL* macro versions.

Well, if they're in GeckoProfiler.h, I want to test them separately.
(Assignee)

Comment 4

3 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/967edd49939b0f42cfddbe488ca9b7044c2b4f53
Bug 1351136 - Write a gtest for basic features of the Gecko Profiler. r=mstange.

Comment 5

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/967edd49939b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 6

3 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/abdb79a72b8177377404ddbc6d9d06c0ffa8f07c
Bug 1351136 (follow-up) - Fix a harmless argument mis-ordering in a call to profiler_get_start_params(). r=me.

Comment 7

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/abdb79a72b81
You need to log in before you can comment on or make changes to this bug.