Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix several tests on Windows. #11048

Merged
merged 2 commits into from
Nov 4, 2022
Merged

Conversation

ScottTodd
Copy link
Member

Most of these were comparing paths using string comparison, which breaks when comparing Unix paths with Windows paths or comparing symlinks with realpaths.

Most of these were comparing paths using string comparison, which breaks when comparing Unix paths with Windows paths or comparing symlinks with realpaths.
@ScottTodd ScottTodd added the platform/windows 🚪 Windows-specific build, execution, benchmarking, and deployment label Nov 4, 2022
Comment on lines -52 to -57
self.assertEqual(
config,
BenchmarkConfig(root_benchmark_dir=os.path.join(self.build_dir.name,
"benchmark_suites"),
benchmark_results_dir=os.path.join(
per_commit_tmp_dir, "benchmark-results"),
Copy link
Member Author

@ScottTodd ScottTodd Nov 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test got messy to update, since BenchmarkConfig calls os.path.realpath in many places.

This led to failures when comparing paths like

'C:\\Users\\runneradmin\\AppData\\Local\\Temp\\tmpm6qhk0oy\\benchmark_suites'
'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmpm6qhk0oy\\benchmark_suites'

Could we just add a way to "assert equal, after canonicalizing all paths"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we stored pathlib.Path objects instead of strings, the comparisons should be safer too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the current solution. We should refactor the tool to take pathlib.Path instead eventually. Can you file a bug?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines -52 to -57
self.assertEqual(
config,
BenchmarkConfig(root_benchmark_dir=os.path.join(self.build_dir.name,
"benchmark_suites"),
benchmark_results_dir=os.path.join(
per_commit_tmp_dir, "benchmark-results"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the current solution. We should refactor the tool to take pathlib.Path instead eventually. Can you file a bug?

@ScottTodd ScottTodd enabled auto-merge (squash) November 4, 2022 22:11
@ScottTodd ScottTodd merged commit b28b2c2 into iree-org:main Nov 4, 2022
@ScottTodd ScottTodd deleted the windows-runtime-tests branch November 4, 2022 22:11
ScottTodd added a commit that referenced this pull request Nov 7, 2022
…1032)

Progress on #11009, depends on
#11048

Changes:
* `build_runtime` + `test_runtime` -> `build_test_runtime` (overhead
from repository cloning, artifact upload, and artifact download was
taking longer than just running the tests from the same job)
* `build_runtime_windows` -> `build_test_runtime_windows`
  * Runs on `managed-windows-cpu` (larger build machine)
* Runs tests, instead of just builds (now that all runtime tests pass on
Windows)
* Runs on presubmit now too, instead of just postsubmit (the build
appears to be stable)

Sample run:
https://github.com/iree-org/iree/actions/runs/3412369869/jobs/5677798847
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform/windows 🚪 Windows-specific build, execution, benchmarking, and deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants