Timing errors on some cargo tests on macOS #5940
Comments
|
@alexcrichton Do you have any opinion on how to proceed? Essentially any test that does:
Where step 2 finishes within 1 second, and step 3 expects the results from 2 to be fresh can fail. Adding sleeps to all these tests doesn't sound great, and will be a problem that will likely plague future tests. The only real option I can think of is to hash the sources, but I think you've said in the past that you don't want to do that? We could back out the mtime change, but it does seem disappointing that touching a file immediately after a build would not count as dirty. |
|
Hm so one thing I've always wanted to perfect in the past is just manually changing the mtimes on files. Could that be done to simulate a sleep perhaps? |
|
That's an interesting idea for some of the tests, but I would be reluctant to try to cover dozens of tests with that kind of logic. One idea I had would be to add hashing only to files that were modified around the time the build starts (say within 2 seconds or so). That would likely avoid performance issues with hashing larger projects, and make cargo resilient against imprecise mtimes. |
|
It's true yeah that'd work! I'm hesitant though to implement such a large change just to fix a few tests. I can try to test out the changes for moving mtimes into the future when I get a chance, but something like hashing should likely be done as its own standalone feature (with its own discussion and whatnot) |
|
So here's my rough idea of how to solve this -- https://gist.github.com/alexcrichton/2cc9269dbd3061d36aa4e3b66ab60a56 The idea there is that let's try to make cargo's test suite actually consistent across all platforms, and let's stop dealing with filesystem weirdness and spurious failures on CI. To that end the test suite unconditionally moves everything (except the target directory) to a known time just before we ever run Cargo. This means that unless further action is taken, the test suite will assume that all files were created at the start of the build and creating files randomly in tests won't actually work. We'd then add a method saying "dear project, please update this file" at which point it would store a new mtime for the file (something like the original mtime plus two seconds). When unconditionally setting the mtime of all files, registered and whitelisted files would be allowed future timestamps. This, I hope, can result in:
That change definitely breaks tons of tests we'd need to fixup. We'd also need to add probably helper methods for "please update this file to these contents". My guess is that Cargo may also need a slightly tweak here and there as well. Does that sound reasonable? Would you be willing to help tackle this? |
…uild, r=alexcrichton Fix path::deep_dependencies_trigger_rebuild often failing in CI A shallow fix for `path::deep_dependencies_trigger_rebuild` in particular as it's failed my PRs often. See #5935 (comment), and #5940 for the bigger picture.
|
Actually I'm almost done finishing off a patch that does this, I'll post for review soon |
|
Ok, I've posted #5953 |
This commit implements a new heavy handed strategy in the test suite to avoid spurious test failures and improve consistency across platforms. Instead of letting mtimes naturally work as they normally do through out tests we force all executions of `cargo` in the tests to, just before `cargo` is run`, deterministically set all mtime values. This way all executions of `cargo` should see the same mtime and hopefully not be susceptible to drift here and there. Additionally tests now have the ability to say that a file needs to be created (to test mtimes and such). This will whitelist the file as having a new mtime which is known to be a large amount of time in the future (to avoid bugs with fast or slow tests). This way we also know exactly that we can bust caches in tests. Finally, Cargo was outfitted with a new change. Whenever Cargo is tracking mtime data it will latch onto the newest mtime (even if it's in the future) for the output. This is meant to handle two cases: * For tests which have files in the far future it allows subsequent rebuilds to work correctly, otherwise since a test never takes hours it looks like there's always a file modified in the future. * When a file is edited during a build this should ensure that the mtime recorded for the build is the mtime *before* the build starts rather than after the build finishes, ensuring that we correctly schedule builds for edited files. Closes rust-lang#5940
Fix slow MacOS Travis issue. OS X 10.13 images on Travis are running very slow and causing timeouts. This PR does two things: - Use OS X 10.12 (`xode9.2`) which is much faster. - Implement a change to the testsuite to handle 1-second resolution mtimes on HFS. When a test executes cargo multiple times, and the first run finishes in under 1 second, the second one will think it needs to rebuild because the mtime of the files equals the mtime of the output. This change forces the mtime of every project to be created 1 second in the past. Tests that are still sensitive to mtimes are adjusted on a case-by-case basis. Closes #6239, Closes #5940
Due to the mtime change in #5919, the following tests fail frequently (some are ~50% on my system) on macOS:
bench::bench_twice_with_build_cmdbench::pass_through_command_linebuild_script::flags_go_into_testsbuild_script::fresh_builds_possible_with_link_libsbuild_script::fresh_builds_possible_with_multiple_metadata_overridesbuild_script::optional_build_dep_and_required_normal_depbuild_script::rebuild_only_on_explicit_pathsfreshness::changing_bin_paths_common_target_features_caches_targetsfreshness::changing_lib_features_caches_targetsfreshness::changing_profiles_caches_targetsfreshness::dont_rebuild_based_on_pluginsfreshness::no_rebuild_transitive_target_depsfreshness::no_rebuild_when_rename_dirfreshness::path_dev_dep_registry_updatesfreshness::reuse_workspace_libfreshness::same_build_dir_cached_packagesfreshness::unused_optional_depinstall::do_not_rebuilds_on_local_installinstall::workspace_uses_workspace_target_dirlocal_registry::simplemetabuild::metabuild_freshpatch::add_ignored_patchpatch::add_patchpatch::new_majorpatch::nonexistentpatch::patch_depends_on_another_patchpatch::patch_gitpatch::patch_in_virtualpatch::replacepatch::unusedpatch::unused_gitpath::custom_target_no_rebuildpath::deep_dependencies_trigger_rebuildpath::nested_deps_recompilepath::no_rebuild_dependencyprofile_targets::profile_selection_benchprofile_targets::profile_selection_buildprofile_targets::profile_selection_build_all_targetsprofile_targets::profile_selection_build_all_targets_releaseprofile_targets::profile_selection_build_releaseprofile_targets::profile_selection_check_all_targetsprofile_targets::profile_selection_check_all_targets_releaseprofile_targets::profile_selection_check_all_targets_testprofile_targets::profile_selection_testprofile_targets::profile_selection_test_releaseregistry::bump_version_dont_update_registryrequired_features::bench_default_featuresrun::run_from_executable_folderrun::specify_namerustc::rustc_fingerprintrustflags::cfg_rustflags_normal_sourcerustflags::cfg_rustflags_precedencetest::bin_does_not_rebuild_teststest::pass_through_command_linetest::selective_testingtool_paths::custom_runnerworkspaces::dep_used_with_separate_featuresI have a fix for some of them, I'll try to finish the rest tomorrow.
EDIT: Looks like it happens to a lot more than I first anticipated.
The text was updated successfully, but these errors were encountered: