MSI: Integration tests #1133
MSI: Integration tests #1133
Conversation
conda-bot
added
the
cla-signed
lrandersson
force-pushed
the
briefcase
branch
from
5d3c3cd to
7073ba6
Compare
.github/workflows/main.yml
Outdated
|
|
||
| - name: Install Briefcase (editable) | ||
| run: | | ||
| pip install -e briefcase |
There was a problem hiding this comment.
Is this still needed? Either way, this should only be installed for Windows.
There was a problem hiding this comment.
You're right, I removed that
tests/test_examples.py
Outdated
| input_path = _example_path("register_envs") | ||
| for installer, install_dir in create_installer(input_path, tmp_path): | ||
| if installer.suffix == ".msi": | ||
| raise Exception("Test for 'register_envs' not yet implemented for MSI") |
There was a problem hiding this comment.
Should these exceptions be NotImplementedError?
There was a problem hiding this comment.
Agree, changed to NotImplementedError
marcoesters
force-pushed
the
briefcase-integration
branch
from
26ee3bb to
a2caeec
Compare
lrandersson
force-pushed
the
briefcase
branch
from
4be5b94 to
6306743
Compare
lrandersson
force-pushed
the
briefcase
branch
from
6306743 to
ad8791d
Compare
constructor/briefcase.py
Outdated
| if "license_file" in info: | ||
| return {"file": info["license_file"]} | ||
| # We cannot return an empty string because that results in an exception on the briefcase side. | ||
| return {"text": "TODO"} |
There was a problem hiding this comment.
We do have a placeholder license: https://github.com/conda/constructor/blob/5eecfa0e8b9a46fa96bc19eb1cbf08a91a25fa52/constructor/nsis/placeholder_license.txt
There was a problem hiding this comment.
Thanks, updated!
tests/test_examples.py
Outdated
| if is_admin(): | ||
| root_dir = Path(os.environ.get("PROGRAMFILES") or r"C:\Program Files") | ||
| else: | ||
| local_dir = os.environ.get("LOCALAPPDATA") or str(Path.home() / r"AppData\Local") |
There was a problem hiding this comment.
Are you using or instead of default values to account for empty strings?
There was a problem hiding this comment.
Correct, updated to follow the convention we use with .get("FOO", <default>)
tests/test_examples.py
Outdated
| process = _execute(cmd, installer_input=installer_input, timeout=timeout, check=check) | ||
| except subprocess.CalledProcessError as e: | ||
| if log_path.exists(): | ||
| # When running on the CI system, it gets misdecodes a UTF-16 log file as UTF-8, |
There was a problem hiding this comment.
| # When running on the CI system, it gets misdecodes a UTF-16 log file as UTF-8, | |
| # When running on the CI system, it tries to decode a UTF-16 log file as UTF-8, |
tests/test_examples.py
Outdated
| if request: # and ON_CI | ||
| # We always need to do this currently since uninstall doesnt work fully | ||
| request.addfinalizer(lambda: shutil.rmtree(str(install_dir), ignore_errors=True)) |
There was a problem hiding this comment.
Why not add rmtree to _run_uninstaller_msi then?
There was a problem hiding this comment.
Initially I didn't want to the interface to *_uninstaller_msi and *_uninstaller_exe to differ, but since this is temporary I've updated it now, I agree it looks more concise this way.
tests/test_examples.py
Outdated
| if request: | ||
| request.addfinalizer( | ||
| lambda: shutil.rmtree(str(install_dir), ignore_errors=True) | ||
| ) |
There was a problem hiding this comment.
Same here - consider adding to _run_uninstaller_msi, especially since this is used in other tests later.
tests/test_examples.py
Outdated
| install_dir: Path, | ||
| timeout: int = 420, | ||
| check: bool = True, | ||
| request: bool = False, |
There was a problem hiding this comment.
request is not a boolean, but a FixtureRequest object: https://docs.pytest.org/en/stable/reference/reference.html#request
However, I would say that it's generally not needed as a finalizer. We can just add rmtree as a clean-up step until the uninstallation works well.
There was a problem hiding this comment.
Ahh good that you caught that, I should not make assumptions based on an expression like if request:, I've updated it 496bf86, let me know if this is what you were thinking. I think this should be OK now, we always invoke rmtree, hopefully no test depended on rmtree not being called.
|
@marcoesters is it possible to restart the CLA/check that failed intermittently? |
marcoesters
merged commit 36a1479
into
conda:briefcase-integration
3 participants
Description
Opened this one separately, this branch and PR depends on #1084. No need to review this for now since the diff will look strange and is more for book-keeping since these changes were pulled out of PR 1084.
Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?