Add support for visualizing Preference markers #2178

hawkinsw · 2019-07-29T23:05:53Z

There is a new type of marker landing in Gecko called Preference.
This marker marks each time a preference is accessed, its name, kind,
type and value. This PR adds support for showing this marker's data
in a tooltip.

Bugzilla side: https://bugzilla.mozilla.org/show_bug.cgi?id=1551313

codecov · 2019-07-29T23:09:14Z

Codecov Report

Merging #2178 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2178      +/-   ##
==========================================
- Coverage   85.73%   85.71%   -0.02%     
==========================================
  Files         200      200              
  Lines       13899    13902       +3     
  Branches     3497     3498       +1     
==========================================
  Hits        11916    11916              
- Misses       1817     1820       +3     
  Partials      166      166

Impacted Files	Coverage Δ
src/components/tooltip/Marker.js	`84.5% <0%> (-1.21%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3067dda...8ddf3a3. Read the comment docs.

codecov · 2019-07-29T23:09:15Z

Codecov Report

Merging #2178 into master will increase coverage by 0.01%.
The diff coverage is 95%.

@@            Coverage Diff             @@
##           master    #2178      +/-   ##
==========================================
+ Coverage   85.86%   85.87%   +0.01%     
==========================================
  Files         200      200              
  Lines       13999    14019      +20     
  Branches     3532     3544      +12     
==========================================
+ Hits        12020    12039      +19     
- Misses       1813     1814       +1     
  Partials      166      166

Impacted Files	Coverage Δ
src/selectors/publish.js	`80% <ø> (ø)`	⬆️
src/reducers/publish.js	`94.39% <ø> (ø)`	⬆️
src/test/fixtures/profiles/gecko-profile.js	`100% <ø> (ø)`	⬆️
src/profile-logic/marker-data.js	`88.86% <100%> (+0.05%)`	⬆️
src/profile-logic/sanitize.js	`98.86% <100%> (+0.09%)`	⬆️
src/components/tooltip/Marker.js	`85.91% <100%> (+0.2%)`	⬆️
src/selectors/profile.js	`96.52% <100%> (+0.12%)`	⬆️
src/components/app/MenuButtons/Publish.js	`73.46% <66.66%> (-0.22%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f083888...8331f41. Read the comment docs.

julienw

Thanks for this addition!
Marking as "request changes" mostly because of questions and adding a test, but otherwise it looks good to me!

By chance, do you have a profile example containing this data so that we can try it out as well?

Also to make sure we don't regress by mistake, it would be a good idea to add a typical preference marker in

profiler/src/test/components/TooltipMarker.test.js

Line 369 in e2971b4

],

and regenerate test snapshots for this test file using yarn test -u TooltipMarker.

Lastly, do you think there could be some PII (personally identifiable information) in this data, that we should give the option to remove when publishing a profile?

src/components/tooltip/Marker.js

hawkinsw · 2019-07-30T17:16:15Z

Thanks for this addition!
Marking as "request changes" mostly because of questions and adding a test, but otherwise it looks good to me!

By chance, do you have a profile example containing this data so that we can try it out as well?

I do, and will upload it here!

Also to make sure we don't regress by mistake, it would be a good idea to add a typical preference marker in

profiler/src/test/components/TooltipMarker.test.js

Line 369 in e2971b4

],

and regenerate test snapshots for this test file using yarn test -u TooltipMarker.

Will do!

Lastly, do you think there could be some PII (personally identifiable information) in this data, that we should give the option to remove when publishing a profile?

Great catch! Yes, we should mark the "Value" as being something that the user does not publish. Is there a way to mark the field as such?

julienw · 2019-07-31T13:48:42Z

Great catch! Yes, we should mark the "Value" as being something that the user does not publish. Is there a way to mark the field as such?

We don't have such automatic behavior unfornately :(

Instead we need to write some adhoc code. Our sanitization code is in the file https://github.com/firefox-devtools/profiler/blob/master/src/profile-logic/sanitize.js. For example this is where we handle network markers:

profiler/src/profile-logic/sanitize.js

Lines 177 to 190 in 1fb607c

    
           // Remove the all network URLs if user wants to remove them. 
        
           if ( 
        
             PIIToBeRemoved.shouldRemoveUrls && 
        
             currentMarker && 
        
             currentMarker.type && 
        
             currentMarker.type === 'Network' 
        
           ) { 
        
             // Remove the URI fields from marker payload. 
        
             markerTable.data[i] = removeNetworkMarkerURLs(currentMarker); 
        
             // Strip the URL from the marker name 
        
             const stringIndex = markerTable.name[i]; 
        
             stringArray[stringIndex] = stringArray[stringIndex].replace(/:.*/, ''); 
        
           }

I think we'll need a new checkbox in our publish panel. Here is how this can be done:

add a new boolean value in

profiler/src/types/profile-derived.js

Lines 170 to 185 in 1fb607c

    
           /** 
        
            * Type that holds the values of personally identifiable information that user 
        
            * wants to remove. 
        
            */ 
        
           export type RemoveProfileInformation = { 
        
             // Remove the given hidden threads if they are provided. 
        
             shouldRemoveThreads: Set<ThreadIndex>, 
        
             // Remove the screenshots if they are provided. 
        
             shouldRemoveThreadsWithScreenshots: Set<ThreadIndex>, 
        
             // Remove the full time range if StartEndRange is provided. 
        
             shouldFilterToCommittedRange: StartEndRange | null, 
        
             // Remove all the URLs if it's true. 
        
             shouldRemoveUrls: boolean, 
        
             // Remove the extension list if it's true. 
        
             shouldRemoveExtensions: boolean, 
        
           };

(eg: shouldRemovePreferenceMarkerValues) and in

profiler/src/types/actions.js

Lines 91 to 101 in 1fb607c

    
           /** 
        
            * This type determines what kind of information gets sanitized from published profiles. 
        
            */ 
        
           export type CheckedSharingOptions = {| 
        
             // The following values are for including more information in a sanitized profile. 
        
             includeHiddenThreads: boolean, 
        
             includeFullTimeRange: boolean, 
        
             includeScreenshots: boolean, 
        
             includeUrls: boolean, 
        
             includeExtension: boolean, 
        
           |};

(eg: includePreferenceMarkerValues).

add a line in

profiler/src/selectors/publish.js

Lines 131 to 143 in 1fb607c

    
           return { 
        
             shouldFilterToCommittedRange: checkedSharingOptions.includeFullTimeRange 
        
               ? null 
        
               : committedRange, 
        
             shouldRemoveUrls: !checkedSharingOptions.includeUrls, 
        
             shouldRemoveThreadsWithScreenshots: new Set( 
        
               checkedSharingOptions.includeScreenshots 
        
                 ? [] 
        
                 : profile.threads.map((_, threadIndex) => threadIndex) 
        
             ), 
        
             shouldRemoveThreads, 
        
             shouldRemoveExtensions: !checkedSharingOptions.includeExtension, 
        
           };

to do the mapping between these 2 values

add some code in https://github.com/firefox-devtools/profiler/blob/master/src/components/app/MenuButtons/Publish.js to add a new line refering to includePreferenceMarkerValues.
change sanitize.js to handle the preference markers as outlined above.

Please tell me if this is too much for you and we can look into handling that for you.
Thanks!

hawkinsw · 2019-08-01T07:10:13Z

Let me know what you think about these changes! I tested the functionality and it "works" but I would definitely appreciate your feedback! Thanks for the documentation that you provided on how to do the sanitization. It was pretty easy thanks to what you sent! Thanks again!

hawkinsw · 2019-08-02T08:50:05Z

@julienw I know that I need to work the code coverage issues, but I'm not exactly sure how I would do that. If you could give me some instructions, I am glad to take care of it! Thanks for your mentorship on this!

julienw · 2019-08-02T15:01:46Z

The changes look good!

For tests, I think the main miss is about the sanitization.

Add a default value for the new property in

profiler/src/test/unit/sanitize.test.js

Lines 13 to 24 in 62f4630

    
           function getRemoveProfileInformation( 
        
             customFields: Object 
        
           ): RemoveProfileInformation { 
        
             return { 
        
               shouldRemoveThreads: new Set(), 
        
               shouldRemoveThreadsWithScreenshots: new Set(), 
        
               shouldRemoveUrls: false, 
        
               shouldFilterToCommittedRange: null, 
        
               shouldRemoveExtensions: false, 
        
               ...customFields, 
        
             }; 
        
           }

in that same file, you can add a new testcase. You can get some inspiration from the testcase should sanitize all the URLs inside network markers. As you'll see you'll also need to get a new marker into our test gecko profile in

profiler/src/test/fixtures/profiles/gecko-profile.js

Line 525 in 62f4630

// INSERT NEW MARKERS HERE

. This may in turn make other tests change their snapshots, but not sure.

Also I'm still waiting for a link and/or a way to capture a profile with preference markers myself, and also a link to the bugzilla bug number in this PR like Markus requested.

What's not clear to me either is whether these markers will be exposed to everybody or if we'll need a custom build or a custom pref or something. If we don't expose them more generally, maybe we don't need the sanitization at all, but that's more your call!

Hope that helps :)

hawkinsw · 2019-08-02T16:33:51Z

https://perfht.ml/316yAfM

At that link is a profile with Preference markers that can be used to do the testing!

hawkinsw · 2019-08-03T06:25:05Z

We have updated the Gecko side of this so that marking preference reads is configurable at run time and not compile time. I think that means that the sanitization addition will not be that confusing to users.

What do you think?

Have a great weekend!

hawkinsw · 2019-08-03T06:47:33Z

https://perfht.ml/2YD1vdX

An updated profile to use to test the FE.

julienw · 2019-08-05T16:55:53Z

So, we discussed my colleagues, and reached an agreement that we want to provide the option, but only when there actually are some preference markers.
Here is how it can be done:

have a selector that returns "are there at least one preference marker". It should probably be in src/selectors/profile.js because we want it to compute from the whole profile (as opposed to the selectors of per-thread that apply to one specific thread) and be something like this:

const getHasPreferenceMarkers: Selector<boolean> = createSelector(
  getThreads,
  threads => {
    return threads.some(({ stringTable, markers }) => {
      const indexForPreference = stringTable.indexForString('Preference');
      return markers.name.some(name => name === indexForPreference);
    });
  }
);

I'll let you adjust the exact condition, but note that here we deal with the raw marker table, not the derived marker list. The structure is a bit different. Especially the array of names is an array of indexes that we need to look up using thread.stringTable, but because they're unique for a string in a thread, we can look it up once at the start and compare the integers, which is likely more performance that comparing strings.

If instead you want to use the type property inside the data array this could look like this:

const getHasPreferenceMarkers: Selector<boolean> = createSelector(
  getThreads,
  threads => {
    return threads.some(({ markers }) => {
      return markers.data.some(data => data && data.type === 'Preference');
    });
  }
);

Then MenuButtonsPublish could take the result of this new selector to decide whether to display the new checkbox.

Again, please tell me if this is too much work for you and I'd be happy to look at it closer and polish it.

Thanks!

hawkinsw · 2019-08-05T21:11:40Z

Thank you for your reply! I will make this change and I will submit a follow-up! Thanks for the detailed explanation!

…

On Mon, Aug 5, 2019 at 12:55 PM Julien Wajsberg ***@***.***> wrote: So, we discussed my colleagues, and reached an agreement that we want to provide the option, but only when there actually are some preference markers. Here is how it can be done: 1. have a selector that returns "are there at least one preference marker". It should probably be in src/selectors/profile.js because we want it to compute from the whole profile (as opposed to the selectors of per-thread that apply to one specific thread) and be something like this: const getHasPreferenceMarkers: Selector<boolean> = createSelector( getThreads, threads => { return threads.some(({ stringTable, markers }) => { const indexForPreference = stringTable.indexForString('Preference'); return markers.name.some(name => name === indexForPreference); }); } ); I'll let you adjust the exact condition, but note that here we deal with the raw marker table, not the derived marker list. The structure is a bit different <https://github.com/firefox-devtools/profiler/blob/355fabf29e69ae79ac2a609ddb78e77c1d1ba9c7/src/types/profile.js#L141-L146>. Especially the array of names is an array of indexes that we need to look up using thread.stringTable, but because they're unique for a string in a thread, we can look it up once at the start and compare the integers, which is likely more performance that comparing strings. If instead you want to use the type property inside the data array this could look like this: const getHasPreferenceMarkers: Selector<boolean> = createSelector( getThreads, threads => { return threads.some(({ markers }) => { return markers.data.some(data => data && data.type === 'Preference'); }); } ); 1. Then MenuButtonsPublish could take the result of this new selector to decide whether to display the new checkbox. Again, please tell me if this is too much work for you and I'd be happy to look at it closer and polish it. Thanks! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2178?email_source=notifications&email_token=ACCP2CUIP3CBAY2RFKOD2MLQDBLRXA5CNFSM4IHW6EYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3SNOWY#issuecomment-518313819>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACCP2CS7CK6HTKFGVUZNAZ3QDBLRXANCNFSM4IHW6EYA> .

julienw · 2019-08-08T15:03:53Z

I removed my review request to clean up my queue a bit, but please put up the review flag again once you're ready! Thanks :)

hawkinsw · 2019-08-08T20:18:10Z

I honestly think that just by putting up a new version of the code you will get notified. If not, here's an @julienw just to make sure :-)

julienw

Thanks for the patch!

The conditional checkbox works fine.
But I don't see the tooltips working anymore and I'm not sure why.

Also there are some things I don't understand properly, especially how the type and the name relate.
My understanding is that type should be Preference but name could change, for example PreferenceRead and PreferenceWrite depending on the action happening in the browser, but tell me what you think (this would be a change in the gecko patch, then).

Also there's a very easy conflict in src/types/markers so it would be nice to rebase on top of latest master.

Note I'll be away next week, but @canaltinova or @gregtatum should be able to move this forward with you.

julienw · 2019-08-09T14:33:52Z

src/components/app/MenuButtons/Publish.js

+                  'includePreferenceValues',
+                  'Include preference values'
+                )
+              : ''}


Suggested change

: ''}

: null}

Happy to change!

julienw · 2019-08-09T14:44:27Z

src/test/components/TooltipMarker.test.js

+          'PreferenceRead',
+          114.9,
+          {
+            type: 'PreferenceRead',


should that be Preference or PreferenceRead?

My understanding is that we should have type as Preference, but the name above being PreferenceRead or PreferenceWrite. Is that it?

julienw · 2019-08-09T14:47:04Z

src/selectors/profile.js

+      const indexForPreferenceString = stringTable.indexForString(
+        'PreferenceRead'
+      );
+      return markers.name.some(name => name === indexForPreferenceString);


See the other comment below too: It's not clear to me what name and type should be, and if we can have several names in the future, but only one type, maybe it's better to use the type here.
What do you think?

julienw · 2019-08-09T14:47:56Z

src/profile-logic/sanitize.js

+        PIIToBeRemoved.shouldRemovePreferenceValues &&
+        currentMarker &&
+        currentMarker.type &&
+        currentMarker.type === 'Preference'


so, this doesn't seem to work now :) at least not in the example you provided.
My feeling (as said in the comments below) is that the type should indeed be Preference and then this code would actually work, but this is not what I see in the profile for now.

Nope, you're right. That sample will no longer work. We changed the name in the gecko profiler patch to make it more obvious that these are preference reads and not some of those type of access. Because of that, the logic changed and older data captures won't work any more. I will post another test data file. Sorry!

julienw · 2019-08-09T14:50:27Z

src/components/tooltip/Marker.js

@@ -545,6 +545,17 @@ function getMarkerDetails(
        );
        break;
      }
+      case 'PreferenceRead': {


same thing here: my understanding is that the type should be Preference :)

Also curiously this doesn't seem to work, see the tooltips in https://deploy-preview-2178--perf-html.netlify.com/public/53ce5ae957fdfeb423d466f435b5bb2e3555cdde/marker-chart/?globalTrackOrder=0-1-2-3-4-5-6&hiddenGlobalTracks=1-2-3-4-5&localTrackOrderByPid=26901-1-2-0~26929-0~27279-0~27073-0~27115-0~27019-0~26966-0-1~&markerSearch=preference&thread=7&v=4

hawkinsw · 2019-08-09T18:22:44Z

Right now the type and the name are the same, as you say. I don't believe that there's much of a use case to generate markers about preference writes. If it's okay with you, I will keep them both the same. In other words, the type of the marker will be PerformanceRead and the name of the marker will be PerformanceRead. Is that okay?

julienw · 2019-08-09T18:26:26Z

If that's ok for you and the reviewer for the gecko patch then it's ok for me !

hawkinsw · 2019-08-09T19:28:34Z

As promised, an updated profile with legitimate testing data!

https://perfht.ml/33oUmO0

hawkinsw · 2019-08-16T19:45:26Z

Thanks for the patch!

The conditional checkbox works fine.
But I don't see the tooltips working anymore and I'm not sure why.

Also there are some things I don't understand properly, especially how the type and the name relate.
My understanding is that type should be Preference but name could change, for example PreferenceRead and PreferenceWrite depending on the action happening in the browser, but tell me what you think (this would be a change in the gecko patch, then).

Also there's a very easy conflict in src/types/markers so it would be nice to rebase on top of latest master.

Note I'll be away next week, but @canaltinova or @gregtatum should be able to move this forward with you.

I just finished rebasing on top of master so that it should merge cleanly. It looks like all the tests pass. Let me know what you think about this version and how we should proceed!

julienw · 2019-08-20T16:20:16Z

I think the main code looks good, but that we need tests to check some of the new functionality, especially that this code won't be exercized manually a lot and we might miss bugs too easily.

I think there are 2 blind spots:

the behavior in the publishing panel: we need a test that checks that the checkbox appears when there's a PreferenceRead marker.
the sanitization behavior: we need a test that checks that the sanitization works as expected.

For 1, I think this would happen in src/test/components/MenuButtons.test.js. Here is a change suggestion:

change setup to take the profile as parameter. Extract the logic const { profile } = getProfileFromTextSamples('A'); profile.meta.updateChannel = updateChannel; to another function.
create another "profile creator" function that would add a marker PreferenceRead. You can use the function addMarkersToThreadWithCorrespondingSamples (from fixtures/profiles/processed-profile.js, where getProfileFromTextSamples is as well).
add a new testcase, following the examples of existing ones like it('matches the snapshot for the opened panel for a nightly profile'

For 2, I think the instructions in #2178 (comment) are still valid.

Tell me if you need anything more. I think we're pretty close!

hawkinsw · 2019-08-21T05:12:50Z

@julienw Thank you very much for the comments on the previous iteration and the instructions for how to add the appropriate tests. I think that the code that is in this PR now should satisfy all your requested changes. Let me know what you think! Thanks again for all your help!

julienw

I'm marking the PR as "approved" but I outlined a change for the new test in sanitize.js that would make it both easier to read and more solid in my opinion.
Once this change is in I think we can merge!

src/test/unit/sanitize.test.js

hawkinsw · 2019-08-22T06:05:33Z

@julienw Thanks for your previous review and your approval of the version prior to this revision. I'd love your feedback on the latest version! I hope that it satisfies your comments.

Thanks again for the review and working with me on this!
Will

julienw

still a few comments about the tests :) I think they're still more complex than they could.

src/test/unit/sanitize.test.js

There is a new type of marker landing in Gecko called Preference. This marker marks each time a preference is accessed, its name, kind, type and value. This PR adds support for showing this marker's data in a tooltip.

julienw · 2019-08-23T10:10:40Z

src/test/unit/sanitize.test.js

+    expect(thread.markers.length).toEqual(1);
+
+    const marker = thread.markers.data[0];
+    // All the conditions have to be checked to make Flow happy.


In tests we sometimes use shortcuts and use any, like expect((marker: any).prefValue).toEqual('preferenceValue'). It's less a big deal in tests to use any especially for expectations.

But I think this is good as it is too so I'll just merge this, thanks :)

Sadly, this is exactly the approach I was trying to avoid. I knew that would work but since it's a total subversion of the type checker, I was trying not to use it. The approach I took feels better to me but the next time I will go with the "easier" version. Thanks again for all the mentorship on landing this!

julienw requested changes Jul 30, 2019

View reviewed changes

src/components/tooltip/Marker.js Outdated Show resolved Hide resolved

julienw self-requested a review August 1, 2019 09:54

julienw removed their request for review August 8, 2019 15:03

julienw self-requested a review August 9, 2019 09:11

julienw requested changes Aug 9, 2019

View reviewed changes

julienw approved these changes Aug 21, 2019

View reviewed changes

src/test/unit/sanitize.test.js Show resolved Hide resolved

julienw reviewed Aug 22, 2019

View reviewed changes

src/test/unit/sanitize.test.js Outdated Show resolved Hide resolved

src/test/unit/sanitize.test.js Outdated Show resolved Hide resolved

Add support for visualizing Preference markers

8331f41

There is a new type of marker landing in Gecko called Preference. This marker marks each time a preference is accessed, its name, kind, type and value. This PR adds support for showing this marker's data in a tooltip.

julienw reviewed Aug 23, 2019

View reviewed changes

julienw merged commit de6a84e into firefox-devtools:master Aug 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for visualizing Preference markers #2178

Add support for visualizing Preference markers #2178

hawkinsw commented Jul 29, 2019 •

edited by canova

codecov bot commented Jul 29, 2019

codecov bot commented Jul 29, 2019 •

edited

julienw left a comment

hawkinsw commented Jul 30, 2019

julienw commented Jul 31, 2019

hawkinsw commented Aug 1, 2019

hawkinsw commented Aug 2, 2019

julienw commented Aug 2, 2019

hawkinsw commented Aug 2, 2019

hawkinsw commented Aug 3, 2019

hawkinsw commented Aug 3, 2019

julienw commented Aug 5, 2019

hawkinsw commented Aug 5, 2019 via email

julienw commented Aug 8, 2019

hawkinsw commented Aug 8, 2019

julienw left a comment

julienw Aug 9, 2019

hawkinsw Aug 9, 2019

julienw Aug 9, 2019

julienw Aug 9, 2019

julienw Aug 9, 2019

hawkinsw Aug 9, 2019

julienw Aug 9, 2019

hawkinsw commented Aug 9, 2019

julienw commented Aug 9, 2019 via email

hawkinsw commented Aug 9, 2019

hawkinsw commented Aug 16, 2019

julienw commented Aug 20, 2019

hawkinsw commented Aug 21, 2019

julienw left a comment

hawkinsw commented Aug 22, 2019

julienw left a comment

julienw Aug 23, 2019

hawkinsw Aug 23, 2019

Add support for visualizing Preference markers #2178

Add support for visualizing Preference markers #2178

Conversation

hawkinsw commented Jul 29, 2019 • edited by canova

codecov bot commented Jul 29, 2019

Codecov Report

codecov bot commented Jul 29, 2019 • edited

Codecov Report

julienw left a comment

Choose a reason for hiding this comment

hawkinsw commented Jul 30, 2019

julienw commented Jul 31, 2019

hawkinsw commented Aug 1, 2019

hawkinsw commented Aug 2, 2019

julienw commented Aug 2, 2019

hawkinsw commented Aug 2, 2019

hawkinsw commented Aug 3, 2019

hawkinsw commented Aug 3, 2019

julienw commented Aug 5, 2019

hawkinsw commented Aug 5, 2019 via email

julienw commented Aug 8, 2019

hawkinsw commented Aug 8, 2019

julienw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkinsw commented Aug 9, 2019

julienw commented Aug 9, 2019 via email

hawkinsw commented Aug 9, 2019

hawkinsw commented Aug 16, 2019

julienw commented Aug 20, 2019

hawkinsw commented Aug 21, 2019

julienw left a comment

Choose a reason for hiding this comment

hawkinsw commented Aug 22, 2019

julienw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkinsw commented Jul 29, 2019 •

edited by canova

codecov bot commented Jul 29, 2019 •

edited