Closed Bug 1393788 Opened 7 years ago Closed 7 years ago

put all jscov artifacts into single zip file

Categories

(Testing :: Code Coverage, enhancement)

Product:
Testing
Component:
Code Coverage
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: ekyle, Assigned: ateoh.dev)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1393788 - Compress all JSDcov artifacts (.json files) into zip files after all tests are completed. .
59 bytes, text/x-review-board-request
sparky
: review+
jmaher
: review+
Details 
Processing jscov files is a little painful because they are split into many small artifacts. I believe zipping them into a single file will make downloading them faster.

[1] TH example: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&filter-searchStr=jsdcov&selectedJob=125884234
[2] jscov example: https://public-artifacts.taskcluster.net/epViDeAoThy7eKs4U2hQAg/0/public/test_info//jscov_1503663935388.json
Attachment #8918604 - Attachment is obsolete: true
Attachment #8918604 - Flags: review?(jmaher)
Attachment #8918604 - Flags: review?(gmierz2)
Comment on attachment 8918605 [details]
Bug 1393788 - Compress all JSDcov artifacts (.json files) into zip files after all tests are completed. .

https://reviewboard.mozilla.org/r/189448/#review194662

this is a great patch to review and see so far along.  I have a lot of little details in the review.  Please update your comment associated with the patch to include why you are doing this and/or what changes you made.  The link to the try job should be in a comment.

Since you are changing the runxpcshelltests.py, you need to test xpcshell on different environments, not just jsdcov, so please run:
./mach try fuzzy -q 'xpcshell' 

that should run xpcshell everywhere.  If there are failures, please retrigger them a couple times to ensure they are not permanent failures.

I look forward to reviewing an updated patch soon!

::: testing/modules/CoverageUtils.jsm:259
(Diff revision 1)
> +    var zipWriter = Cc["@mozilla.org/zipwriter;1"]
> +                  .createInstance(Components.interfaces.nsIZipWriter);
> +    zipWriter.open(zipFile, openFlags);
> +
> +    // Iterate through all jscov artifacts
> +    dump("Iterate through all JSCov artifacts. \n");

this dump seems like an unnecessary dump statement for running in production.

::: testing/modules/CoverageUtils.jsm:261
(Diff revision 1)
> +    zipWriter.open(zipFile, openFlags);
> +
> +    // Iterate through all jscov artifacts
> +    dump("Iterate through all JSCov artifacts. \n");
> +    var success_zip = false;
> +    for (var i = 0; i < this._jscovFiles.length; i++){

please focus on consistent style, n this case you should have an extra space near the end of the line between '){'

::: testing/modules/CoverageUtils.jsm:265
(Diff revision 1)
> +    var success_zip = false;
> +    for (var i = 0; i < this._jscovFiles.length; i++){
> +      var jscov_filepath = this._prefix + "/" + this._jscovFiles[i];
> +      var jscov_file = new FileUtils.File(jscov_filepath);
> +
> +      if (jscov_file.exists()){

another case of needing an extra space

::: testing/modules/CoverageUtils.jsm:279
(Diff revision 1)
> +          dump ("Warning: Cannot find JSCov artifacts: " + jscov_filepath + "\n");
> +      }
> +    }
> +    zipWriter.close();
> +
> +    if (!success_zip){

and another space needed!

::: testing/modules/CoverageUtils.jsm:282
(Diff revision 1)
> +    zipWriter.close();
> +
> +    if (!success_zip){
> +        OS.File.remove(zip_filepath);
> +    }
> +}

if we do succeed, do we have a need to delete the raw files that are now in the .zip file?

::: testing/xpcshell/runxpcshelltests.py:1044
(Diff revision 1)
>            Simple wrapper to get the absolute path for a given directory name.
>            On a remote system, we need to overload this to work on the remote filesystem.
>          """
>          return os.path.abspath(dirname)
>  
> +    def launchProcess(self, cmd, stdout, stderr, env, cwd, timeout=None):

this looks like a direct copy of the method from class XPCShellTestThread(Thread):
http://searchfox.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#

Can you at least update the comment to ensure that we know why this is implemented (i.e. We are launching processes for post task processing (code coverage))

::: testing/xpcshell/runxpcshelltests.py:1079
(Diff revision 1)
> +            proc.communicate()
> +
> +            # Delete compressed JSCov artifacts.
> +            for filename in os.listdir(self.jscovdir):
> +                if filename.startswith("jscov") and filename.endswith(".json"):
> +                    os.remove( os.path.join(self.jscovdir, filename) )

nit: please remove the extra spaces around the parameter inside of os.remove(), it should read:
os.remove(os.path.join(...))

::: testing/xpcshell/runxpcshelltests.py:1080
(Diff revision 1)
> +
> +            # Delete compressed JSCov artifacts.
> +            for filename in os.listdir(self.jscovdir):
> +                if filename.startswith("jscov") and filename.endswith(".json"):
> +                    os.remove( os.path.join(self.jscovdir, filename) )
> +                    

nit, blank line with whitespace, this should just be a blank line
Attachment #8918605 - Flags: review?(jmaher) → review+
Comment on attachment 8918605 [details]
Bug 1393788 - Compress all JSDcov artifacts (.json files) into zip files after all tests are completed. .

https://reviewboard.mozilla.org/r/189448/#review194664

I accidentally did r+, this is really close to r+ though!
Attachment #8918605 - Flags: review+ → review-
Great progress so far Alex! I have a few things that need to be changed here (some of jmaher's comments may no longer be relevant after these changes, although they are still things you need to watch for in the next patch). Also, when I said that you need to provide a TH link, I meant that it should be provided through a comment like this one on bugzilla, not through the patch comment, sorry. :)

First, the mochitest suites are still packaged in different zip files. And second, you've provided two different mechanisms to zip all the files, and we should (if we can) use only one.

It would be better for us to have a single method that zips all the json files into a single zip. We can do this by setting an environment variable here: https://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py?q=testing%2Fxpcshell%2Frunxpcshelltests.py&redirect_type=direct#1257

i.e. os.environ["JSDCOV_OUTPUT_DIR"] = "<PATH/TO/JSONS>"

(We would need to also do this in the mochitest's runtests function, [3]).

We can then use this in [1], and at the end of any test suite (post-action listener), we can simply collect all the jsons within that folder into a single zip. With this, we won't have to modify CoverageUtils.jsm and all you will need to do is run your 'compressJSCovArtifacts' function on the folder within the post-action listener function '_package_coverage_data'. Another benefit of this is that you won't need to reimplement an already existing function ('launchProcess') as we already have access to something similar in 'codecoverage.py' [2]. 

[1]: https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/codecoverage.py?q=codecoverage.py&redirect_type=direct#91
[2]: https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/codecoverage.py?q=codecoverage.py&redirect_type=direct#118-119
[3]: https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#2475
Comment on attachment 8918605 [details]
Bug 1393788 - Compress all JSDcov artifacts (.json files) into zip files after all tests are completed. .

https://reviewboard.mozilla.org/r/189448/#review194668

See comment 5 in the bug 1393788.
Attachment #8918605 - Flags: review?(gmierz2) → review-
And as always, let us know if you need any clarifications or have any questions! :)
Hi Greg, thanks for the detailed comments above! They are very clear and helpful. 
I have one question about that approach. I thought all files are uploaded to s3 prior to the execution of codecoverage.py. Hence we need to zip the artifacts prior to codecoverage.py (i.e. with CoverageUtils/ runtests scripts) Is this not true? 

Thanks,
Alex Teoh
Flags: needinfo?(gmierz2)
Good question! Parts of codecoverage.py are run right at the end of runtests and right before it's execution (have a look into post/pre action listeners) - before the artifacts are uploaded. It's purpose is to package code coverage data after a chunk of a suite is finished.
Flags: needinfo?(gmierz2)
Hi Greg,

I set my environ variable in both mochitests and xpcshelltests execution scripts, but I cannot access that variable in PostScriptAction of codecoverage.py for both tests. I also tried declaring my envrion variable in run_test_harness(), which executes runTests(). To help debugging this, I print out all environ variables in different stages. I can see my envrion variable when executing the tests (i.e. runTests), but not in Pre/PostScriptAction in codecoverage.py. 

I think I need to setup my envrion variable earlier. Can you advise? Also, when it says "run-tests" action in PostScriptAction, which function is it actually listening to? 

Current Changeset (messy): https://hg.mozilla.org/try/rev/1eb75b682f50cab12c0a3cd230432131da1ce06e
Output example (search for "alex" to get my debug printout): https://public-artifacts.taskcluster.net/DkBgjkKPQFieijoYzJ1Jzw/0/public/logs//log_raw.log
TH Link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dacdd5f707b0e610c074b6bfed4e3a651bd447d7
Flags: needinfo?(gmierz2)
Good progress! Hmm, it seems like we won't be able to do it this way because the Postscriptaction function resides in another process (or is not invoked correctly to save that environment variable). Try following how we implement MOZ_CODE_COVERAGE to see if you can implement a new variable that can be used everywhere that way. Set it to true/1 for now and see if you can read it everywhere, after that we'll have to make some modifications to how we run linux64-jsdcov.

I'm not 100% sure of which functions the action 'run-tests' specifies - jmaher would you be able to tell us? Also, do you have any thoughts about how we can setup a new environment variable? I'm thinking that if we follow MOZ_CODE_COVERAGE's implementation we could do it but I might be missing something here.
Flags: needinfo?(gmierz2) → needinfo?(jmaher)
in mozharness 'run-tests' specifies an action (which is basically a function call), in this case it is to run the tests, you can see an example here in desktop unittests:
http://searchfox.org/mozilla-central/source/testing/mozharness/scripts/desktop_unittest.py#196

and the corresponding run-tests -> run_tests:
http://searchfox.org/mozilla-central/search?q=run_tests&path=testing%2Fmozharness
Flags: needinfo?(jmaher)
Turns out the file Joel pointed out (desktop_unittest.py) is the right place to insert the environment variable. It will be effective for both mochitest and xpcshell tests. Thanks to this, the new patch has become very simple. 

Greg, I also tried your suggestion. Unfortunately, I couldn't get the environment variable to show up in PostScriptAction.
Comment on attachment 8918605 [details]
Bug 1393788 - Compress all JSDcov artifacts (.json files) into zip files after all tests are completed. .

https://reviewboard.mozilla.org/r/189448/#review198948

this is much simpler- really close to a r+, two issues and I would be happy to review again once those are addressed/answered.

::: testing/mozharness/scripts/desktop_unittest.py:443
(Diff revision 2)
>  
>              if suite in ('browser-chrome-coverage', 'xpcshell-coverage',
>                           'mochitest-devtools-chrome-coverage', 'plain-chunked-coverage'):
>                  base_cmd.append('--jscov-dir-prefix=%s' %
>                                  dirs['abs_blob_upload_dir'])
> +                os.environ['JSDCOV_OUTPUT_DIR'] = dirs['abs_blob_upload_dir']

I know there is prior art here of the base_cmd.append(...), but do we need both the cmdline and env var to have the dir defined?  

Do we need to conditionally define this- it appears to be defined for all cases (maybe a bug in the existing code)?

::: testing/mozharness/scripts/desktop_unittest.py
(Diff revision 2)
>  
>                  if self.config['enable_stylo']:
>                      env['STYLO_FORCE_ENABLED'] = '1'
>                  if self.config['disable_stylo']:
>                      env['STYLO_FORCE_DISABLED'] = '1'
> -

we shouldn't delete a blank line here- I think it makes it more readable to have the additional blank line.
Attachment #8918605 - Flags: review?(jmaher) → review-
Comment on attachment 8918605 [details]
Bug 1393788 - Compress all JSDcov artifacts (.json files) into zip files after all tests are completed. .

https://reviewboard.mozilla.org/r/189448/#review199182

::: commit-message-2cd37:1
(Diff revision 2)
> +[mq]: bug1393788.patch

You'll have to remove this from the commit message and move line 3 up here. Leave a comment about what this patch does at line 3 instead.
Attachment #8918605 - Flags: review?(gmierz2) → review-
I spoke with Alex today during the meeting and he will work on removing the 'os.environ' call in favor of a new flag here: https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/codecoverage.py#16
Updated with the latest approach. It works perfectly. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=010dc3b36154e0f3ca8fe1020930c47c66b4c360
Comment on attachment 8918605 [details]
Bug 1393788 - Compress all JSDcov artifacts (.json files) into zip files after all tests are completed. .

https://reviewboard.mozilla.org/r/189448/#review199212

just the one nit

::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:129
(Diff revision 3)
> +            for filename in os.listdir(jsdcov_dir):
> +                if filename.startswith("jscov") and filename.endswith(".json"):
> +                    os.remove(os.path.join(jsdcov_dir, filename))
> +
> +            self.info("Completed JSDCov artifacts compression! ")
> +            self.info("Here's the zip file: " + zipFile)

This statement isn't the most informative for end users, maybe say:
"Path to JSDCov compressed artifacts: " + zipFile
Attachment #8918605 - Flags: review?(jmaher) → review+
Comment on attachment 8918605 [details]
Bug 1393788 - Compress all JSDcov artifacts (.json files) into zip files after all tests are completed. .

https://reviewboard.mozilla.org/r/189448/#review199226

::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:9
(Diff revision 3)
>  # You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  import os
>  import shutil
>  import tempfile
> +import time

Remove this after removing the timestamp creation also.

::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:114
(Diff revision 3)
>      def _package_coverage_data(self, action, success=None):
> +        if self.jsd_code_coverage_enabled:
> +            # Setup the command for compression
> +            dirs = self.query_abs_dirs()
> +            jsdcov_dir = dirs['abs_blob_upload_dir']
> +            timeStamp = str(int(time.time() * 1000))

No need to append a timestamp to the name since there is only ever one zip file that will exist. 'jsdcov_artifacts.zip' is good enough without the time.

::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:120
(Diff revision 3)
> +            zipFilename = "jsdcov_artifacts_" + timeStamp + ".zip"
> +            zipFile = os.path.join(jsdcov_dir, zipFilename)
> +            command = ["zip", "-r", zipFile, ".", "-i", "jscov*.json"]
> +
> +            self.info("Begin to compress JSDCov artifacts... ")
> +            # command is guaranteed to be executed before run_command returns

This comment is not useful here, you can remove it. Or say something like, 'Zip together all individual jsdcov json files'.
Attachment #8918605 - Flags: review?(gmierz2) → review-
Good work Alex! Just fix these few nits and we'll be good to go.
Comment on attachment 8918605 [details]
Bug 1393788 - Compress all JSDcov artifacts (.json files) into zip files after all tests are completed. .

https://reviewboard.mozilla.org/r/189448/#review199230

I missed a couple things.

::: commit-message-2cd37:3
(Diff revision 3)
> +Bug 1393788 - Compress all jscov artifacts (.json files) into one .zip file after all tests are completed. r?jmaher, r?gmierz.
> +
> +[mq]: bug1393788.patch

I just noticed this. You need to add a comment here that describes what your patch does. You have to remove this line starting with '[mq]:...' and leave a comment about what your patch accomplishes.

::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:119
(Diff revision 3)
> +            timeStamp = str(int(time.time() * 1000))
> +            zipFilename = "jsdcov_artifacts_" + timeStamp + ".zip"
> +            zipFile = os.path.join(jsdcov_dir, zipFilename)
> +            command = ["zip", "-r", zipFile, ".", "-i", "jscov*.json"]
> +
> +            self.info("Begin to compress JSDCov artifacts... ")

nit: 'Beginning compression of JSDCov artifacts...' sounds better, but it's up to you.
Comment on attachment 8918605 [details]
Bug 1393788 - Compress all JSDcov artifacts (.json files) into zip files after all tests are completed. .

https://reviewboard.mozilla.org/r/189448/#review199212

> This statement isn't the most informative for end users, maybe say:
> "Path to JSDCov compressed artifacts: " + zipFile

Sure! Thanks for the tip.
Comment on attachment 8918605 [details]
Bug 1393788 - Compress all JSDcov artifacts (.json files) into zip files after all tests are completed. .

https://reviewboard.mozilla.org/r/189448/#review199226

> No need to append a timestamp to the name since there is only ever one zip file that will exist. 'jsdcov_artifacts.zip' is good enough without the time.

Sure. But when we get to the ETL pipeline, will these artifacts be located in the same folder or in different sub-folders?

> This comment is not useful here, you can remove it. Or say something like, 'Zip together all individual jsdcov json files'.

Okay. I wrote this because I ran into some implementation of "run_command" that spawns sub-process and returns before completion of sub-process.
(In reply to Alex from comment #26)
> Comment on attachment 8918605 [details]
> Bug 1393788 - Compress all jscov artifacts (.json files) into one .zip file
> after all tests are completed. .
> 
> https://reviewboard.mozilla.org/r/189448/#review199226
> 
> > No need to append a timestamp to the name since there is only ever one zip file that will exist. 'jsdcov_artifacts.zip' is good enough without the time.
> 
> Sure. But when we get to the ETL pipeline, will these artifacts be located
> in the same folder or in different sub-folders?

You'll have to ask :ekyle about that but there shouldn't be any problems because we ingest other artifacts from 'linux64-ccov' that are not timestamped. The only reason the json files have timestamps here is because there are more than one of them. 

> 
> > This comment is not useful here, you can remove it. Or say something like, 'Zip together all individual jsdcov json files'.
> 
> Okay. I wrote this because I ran into some implementation of "run_command"
> that spawns sub-process and returns before completion of sub-process.

There are more than just two definitions actually: https://dxr.mozilla.org/mozilla-central/search?q=def+run_command&redirect=true

It's a good idea but you don't have to worry about this, you are using this definition: https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/base/script.py#1295
Recent update contains some code clean up and better info messages. Functionality is not modified. 
TH Link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=27817ded3366e7354b51f5a57a926c3878c6a172
Comment on attachment 8918605 [details]
Bug 1393788 - Compress all JSDcov artifacts (.json files) into zip files after all tests are completed. .

https://reviewboard.mozilla.org/r/189448/#review199230

> I just noticed this. You need to add a comment here that describes what your patch does. You have to remove this line starting with '[mq]:...' and leave a comment about what your patch accomplishes.

Got it.
Comment on attachment 8918605 [details]
Bug 1393788 - Compress all JSDcov artifacts (.json files) into zip files after all tests are completed. .

https://reviewboard.mozilla.org/r/189448/#review194662

Thanks for the detailed review! I will come back with an update containing your inputs soon.

> if we do succeed, do we have a need to delete the raw files that are now in the .zip file?

This is done in line 271 with OS.File.remove(jscov_filepath)
Comment on attachment 8918605 [details]
Bug 1393788 - Compress all JSDcov artifacts (.json files) into zip files after all tests are completed. .

https://reviewboard.mozilla.org/r/189448/#review200464

::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:117
(Diff revision 4)
> +            jsdcov_dir = dirs['abs_blob_upload_dir']
> +            zipFile = os.path.join(jsdcov_dir, "jsdcov_artifacts.zip")
> +            command = ["zip", "-r", zipFile, ".", "-i", "jscov*.json"]
> +
> +            self.info("Beginning compression of JSDCov artifacts...")
> +            # command is guaranteed to be executed before run_command returns

You still need to remove this comment. It's not useful here and we've discussed this.
Attachment #8918605 - Flags: review?(gmierz2) → review-
Comment on attachment 8918605 [details]
Bug 1393788 - Compress all JSDcov artifacts (.json files) into zip files after all tests are completed. .

https://reviewboard.mozilla.org/r/189448/#review200464

> You still need to remove this comment. It's not useful here and we've discussed this.

Hi Greg, not sure if you have seen my reply above. 
Subprocess is forked to execute unix command. In some case, the helper function doesn't wait for the subprocess to finish before returning. I debugged this issue before in earlier version of the code, so I left this comment here. 
With this, if you still don't think this comment is necessary, then I will remove it.

Thanks,
Alex
Comment on attachment 8918605 [details]
Bug 1393788 - Compress all JSDcov artifacts (.json files) into zip files after all tests are completed. .

https://reviewboard.mozilla.org/r/189448/#review200464

> Hi Greg, not sure if you have seen my reply above. 
> Subprocess is forked to execute unix command. In some case, the helper function doesn't wait for the subprocess to finish before returning. I debugged this issue before in earlier version of the code, so I left this comment here. 
> With this, if you still don't think this comment is necessary, then I will remove it.
> 
> Thanks,
> Alex

I replied to that at the end of comment 27 in the bug. It's not an issue here so you can remove it.
Comment on attachment 8918605 [details]
Bug 1393788 - Compress all JSDcov artifacts (.json files) into zip files after all tests are completed. .

https://reviewboard.mozilla.org/r/189448/#review200586

Good work Alex! This is good to go for me.
Attachment #8918605 - Flags: review?(gmierz2) → review+
Comment on attachment 8918605 [details]
Bug 1393788 - Compress all JSDcov artifacts (.json files) into zip files after all tests are completed. .

https://reviewboard.mozilla.org/r/189448/#review200592
I am unable to land the commit via mozreview, :ryanvm, can you land this?
Flags: needinfo?(ryanvm)
Not while MozReview is showing 10 unresolved issues. Autoland can't push this until they're all marked as resolved.
Assignee: nobody → ateoh.dev
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/82d46d307abf
Compress all JSDcov artifacts (.json files) into zip files after all tests are completed. r=gmierz,jmaher.
Hi Ryan,
I've marked all the remaining issues as resolved. Those are issues in earlier versions of the code, so they are no longer relevant here. Sorry about that. (Looks like autoland has already gone through)

Thanks,
Alex
https://hg.mozilla.org/mozilla-central/rev/82d46d307abf
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: