Conversation
There was a problem hiding this comment.
Pull request overview
This PoC adjusts the Python e2e test infrastructure to run more directly (eg via hatch run ...) without relying on ./tests.sh/ctest, primarily by making binary path resolution work when the binaries live outside the current working directory.
Changes:
- Resolve the “combined binary” entry-point via
binary_dirinCCFRemoteand avoid copying the same file twice for 7.x. - Thread
binary_dirthrough initial node creation inNetwork. - Change the default
--binary-dirCLI argument to../build.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/infra/remote.py |
Adjusts how the executed binary path is derived for 7.x and how startup files are packaged for remotes. |
tests/infra/network.py |
Ensures binary_dir is explicitly forwarded when creating nodes. |
tests/infra/e2e_args.py |
Updates the default binary directory to match a “run from tests/” workflow. |
You can also share your feedback on Copilot code review. Take the survey.
| # 7.x releases combined binaries and removed the separate cchost entry-point | ||
| if major_version is None or major_version >= 7: |
There was a problem hiding this comment.
The decision to override self.BIN based solely on major_version >= 7 conflicts with the later version-based CLI logic which still adds --enclave-file for versions <= 7.0.0-dev1. For tags like ccf-7.0.0-dev0/dev1 (see CHANGELOG), this would make the node try to execute enclave_file as the host binary even though these versions still require the cchost entry-point plus --enclave-file. Consider selecting the entry-point based on the parsed full version (the same threshold used for the --enclave-file flag) rather than major_version alone, so 7.0.0-dev0/dev1 keep using cchost.
| # 7.x releases combined binaries and removed the separate cchost entry-point | |
| if major_version is None or major_version >= 7: | |
| # 7.x releases combined binaries and removed the separate cchost entry-point. | |
| # Use the same full-version threshold as the CLI logic which drops --enclave-file: | |
| # only versions strictly greater than 7.0.0-dev1 use the combined binary as entry-point. | |
| if "version" in locals() and version is not None: | |
| if version > Version("7.0.0-dev1"): | |
| self.BIN = infra.path.build_bin_path( | |
| enclave_file, binary_dir=binary_dir | |
| ) | |
| elif major_version is None or major_version >= 7: | |
| # Fallback for callers which only provide major_version |
| "-b", | ||
| "--binary-dir", | ||
| help="Path to CCF binaries (cchost, scurl, keygenerator)", | ||
| default=".", | ||
| default="../build", | ||
| ) |
There was a problem hiding this comment.
Changing the default --binary-dir to ../build makes the CLI default dependent on the caller's current working directory being tests/ (or similar). This is likely to break existing workflows that invoke these scripts from the build directory (where -b . used to work without specifying it) or from an installed tree. Consider deriving the default from the script location and/or probing common candidate directories (e.g. prefer . if it contains keygenerator.sh, else fall back), rather than hard-coding a relative path.
Headline: no
./tests.sh, noctest. More amenable to things like #7709.Next questions:
tdnfwe could use. I thoughthatchwas, but it was just plugins. So you have topip install hatch. But then that creates the venv etc for you. Could we do this withuvinstead?hatch runseems to do it for free? Maybe!