Closed Bug 1451319 Opened 6 years ago Closed 6 years ago

Windows builds uploading .dmp files from mozcrash unit tests

Categories

(Testing :: Mozbase, defect, P1)

Product:
Testing
Component:
Mozbase
Version:
Version 3
Type:
defect

Tracking

(firefox-esr52 unaffected, firefox59 unaffected, firefox60 unaffected, firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 --- fixed

People

(Reporter: ted, Assigned: whimboo)

References

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1451319 - Unset MINIDUMP_SAVE_PATH for mozcrash unit tests.
59 bytes, text/x-review-board-request
davehunt
: review+
Details 
Yes, this looks like a regression from bug 1441287. I will have a look.
Assignee: nobody → hskupin
Blocks: 1441287
Status: NEW → ASSIGNED
status-firefox60: --- → unaffected
status-firefox61: --- → affected
Keywords: regression
Priority: -- → P1
Those tests use the `tmpdir` pytest fixture, which should have created those files at a temporary location, which also automatically should be removed once the test finishes. So why does that happen?

Here the line in the log which logs the upload of one of those files:

> [taskcluster 2018-04-04T12:26:56.156Z] Uploading artifact public/build/4e865784-b0db-4d29-9e58-7db5a0426ec2.dmp from file public\build\4e865784-b0db-4d29-9e58-7db5a0426ec2.dmp with content encoding "gzip", mime type "application/octet-stream" and expiry 2019-04-04T11:36:09.146Z

It means that the files are placed in `public/build/`.

The TMP/TEMP environment variables are set to:

> 11:44:01     INFO -  'TEMP': 'C:\\Users\\task_1522838894\\AppData\\Local\\Temp',
> 11:44:01     INFO -  'TMP': 'C:\\Users\\task_1522838894\\AppData\\Local\\Temp',

So why does tmpfile pick up `public/build` instead? Dave, do you have an idea?
Flags: needinfo?(dave.hunt)
So it could happen if the basetemp directory gets changed:
https://docs.pytest.org/en/latest/tmpdir.html#base-temporary-directory

> pytest --basetemp=mydir

Also this doesn't seem to happen for the mozbase tests as run via the `mb` job on Linux:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8eec06866ec979e059f8e95a0f9d6fae46ad9159&filter-tier=1&filter-tier=2&filter-tier=3&filter-searchStr=python&selectedJob=171128254

Whereby we might not collect files for upload there.

Andrew, do you know if we modify the basetemp location for python tests on Windows build jobs?
Flags: needinfo?(ahalberstadt)
Versions in use on Windows: platform win32 -- Python 2.7.14, pytest-3.1.3, py-1.4.34, pluggy-0.4.0
No, I don't think so:
https://searchfox.org/mozilla-central/source/config/mozunit/mozunit/mozunit.py#234

We do set a PYTHON_TEST_TMP environment variable in python/mach_commands.py, but afaict that isn't something pytest honours nor do any of the mozbase tests make use of it.
Flags: needinfo?(ahalberstadt)
Oh, that could be it and would make totally sense. We should set this environment variable to another temporary folder instead. Thanks Ted!
Flags: needinfo?(dave.hunt)
I can actually reproduce when running the following command:

> MINIDUMP_SAVE_PATH=minidumps/ mach python-test testing/mozbase/mozcrash

Then the `minidumps` folder is filled with minidump files.
Comment on attachment 8965258 [details]
Bug 1451319 - Unset MINIDUMP_SAVE_PATH for mozcrash unit tests.

https://reviewboard.mozilla.org/r/233968/#review239606

::: testing/mozbase/mozcrash/tests/conftest.py:93
(Diff revision 1)
>  
>      monkeypatch.setattr(mozcrash.mozcrash.subprocess, 'Popen', MockPopen)
> +
> +
> +@pytest.fixture
> +def no_minidump_save_path():

The `check_for_crashes` fixture could just use the `monkeypatch` fixture to delete the environment variable. It will be restored automatically.

```python
@pytest.fixture
def check_for_crashes(tmpdir, monkeypatch, stackwalk):
    monkeypatch.delenv('MINIDUMP_SAVE_PATH', raising=False)
    ...
```

See https://docs.pytest.org/en/latest/reference.html#_pytest.monkeypatch.MonkeyPatch.delenv
Attachment #8965258 - Flags: review?(dave.hunt) → review-
Comment on attachment 8965258 [details]
Bug 1451319 - Unset MINIDUMP_SAVE_PATH for mozcrash unit tests.

https://reviewboard.mozilla.org/r/233968/#review239606

> The `check_for_crashes` fixture could just use the `monkeypatch` fixture to delete the environment variable. It will be restored automatically.
> 
> ```python
> @pytest.fixture
> def check_for_crashes(tmpdir, monkeypatch, stackwalk):
>     monkeypatch.delenv('MINIDUMP_SAVE_PATH', raising=False)
>     ...
> ```
> 
> See https://docs.pytest.org/en/latest/reference.html#_pytest.monkeypatch.MonkeyPatch.delenv

Oh, sweet. I have known there might be a better way in doing that. Will be fixed in my next update. Thanks!
Comment on attachment 8965258 [details]
Bug 1451319 - Unset MINIDUMP_SAVE_PATH for mozcrash unit tests.

https://reviewboard.mozilla.org/r/233968/#review239614


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/mozbase/mozcrash/tests/conftest.py:90
(Diff revision 3)
>  
>          def wait(self):
>              return self.returncode
>  
>      monkeypatch.setattr(mozcrash.mozcrash.subprocess, 'Popen', MockPopen)
> +

Error: Blank line at end of file [flake8: W391]
Comment on attachment 8965258 [details]
Bug 1451319 - Unset MINIDUMP_SAVE_PATH for mozcrash unit tests.

https://reviewboard.mozilla.org/r/233968/#review239616
Attachment #8965258 - Flags: review?(dave.hunt) → review+
Comment on attachment 8965258 [details]
Bug 1451319 - Unset MINIDUMP_SAVE_PATH for mozcrash unit tests.

https://reviewboard.mozilla.org/r/233968/#review239614

> Error: Blank line at end of file [flake8: W391]

Not sure that I understand what's wrong here. The latest update lints just fine for me.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aecbdcadaf96
Unset MINIDUMP_SAVE_PATH for mozcrash unit tests. r=davehunt
Backout by toros@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e5b89dcca72e
Backed out changeset aecbdcadaf96 for linting opt failures /builds/worker/checkouts/gecko/testing/mozbase/mozcrash/tests/conftest.py:90 on a CLOSED TREE
Comment on attachment 8965258 [details]
Bug 1451319 - Unset MINIDUMP_SAVE_PATH for mozcrash unit tests.

https://reviewboard.mozilla.org/r/233968/#review239614

> Not sure that I understand what's wrong here. The latest update lints just fine for me.

Sorry, I checked that locally when I had a different bookmark checked out. Totally my fault.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2c66e9c2d4d0
Unset MINIDUMP_SAVE_PATH for mozcrash unit tests. r=davehunt
https://hg.mozilla.org/mozilla-central/rev/2c66e9c2d4d0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: