1351136 - Write a gtest for basic features of the Gecko Profiler

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[Nicholas Nethercote [inactive]]

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_*().

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Write a gtest for basic features of the Gecko Profiler

This required a tweak to DoNativeBacktrace() to work around an ASAN false
positive.

Comment 2

[Markus Stange [:mstange]]

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.

Comment 3

[Nicholas Nethercote [inactive]]

> 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.

Comment 4

[Nicholas Nethercote [inactive]]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/967edd49939b

Comment 6

[Nicholas Nethercote [inactive]]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/abdb79a72b81