Tests modification for windows CI #9671

cheme · 2018-09-28T13:32:47Z

Running tests on windows, fixing some path to string conversion and others windows specifics :

change of duration value in corpus_inaccessible because of substract overflow happening on windows
right color configuration for test_command_signer_new_token
extend delay for keyserver test.
deactivate guards_delete_folder due to error on removal when guards is dropped
deactivate should_allow_if_authorization_is_correct (timeout error is thrown)

At the time those changes allows the test to run on windows 10 vm, but on the windows CI there is still a segfault on specific tests (probably rocksdb related or related to temp path issue) :

test snapshot::service::tests::cannot_finish_with_invalid_chunks ... error: test failed, to rerun pass '--lib'
/c/r/builds/f8bd3cf5/0/parity/parity-ethereum/test.sh: line 59:  2224 Segmentation fault      cargo test $OPTIONS --features "$FEATURES" --all $1 -- --test-threads 1

…tests)

parity-cla-bot · 2018-09-28T13:32:50Z

It looks like @cheme signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

NikVolf · 2018-09-28T19:57:55Z

ethstore/src/accounts_dir/vault.rs

@@ -310,8 +310,10 @@ mod test {

 	#[test]
 	fn make_vault_dir_path_succeeds() {
-		assert_eq!(make_vault_dir_path("/home/user/parity", "vault", true).unwrap().to_str().unwrap(), "/home/user/parity/vault");
-		assert_eq!(make_vault_dir_path("/home/user/parity", "*bad-name*", false).unwrap().to_str().unwrap(), "/home/user/parity/*bad-name*");
+    use std::path::Path;


spaces here

debris · 2018-10-01T13:00:35Z

ethcore/light/src/cache.rs

@@ -186,7 +186,7 @@ mod tests {

 		{
 			let corpus_time = &mut cache.corpus.as_mut().unwrap().1;
-			*corpus_time = *corpus_time - Duration::from_secs(6 * 3600);
+			*corpus_time = *corpus_time - Duration::from_secs(5 * 3600);


why is this change needed?

It substracts overflow otherwhise (of course that is only happening on windows). That looks like a windows bug/inconsistency with duration.

🤦‍♂️ please describe it in the comment

debris · 2018-10-01T13:02:37Z

ethcore/src/snapshot/tests/service.rs

+// on windows the guards deletion (remove_dir_all)
+// is not happening (error directory is not empty).
+// So the test is disabled until windows api behave.
+#[cfg(not(target_os = "windows"))]


Can you tell something more about the error returned? Why is it not happening? According to the documentation

Removes a directory at this path, after removing all its contents. Use carefully!

It should happen

The error is a simple 'directory is not empty', looking at rust-lang/rust#29497 which exists since 2015, I may have been a bit optimistic in saying that there will be a fix someday, we may probably want to use https://crates.io/crates/remove_dir_all .

Ok, I ran a test with the 'remove_dir_all' crate, and it is not better : we obtain an "os 32" error "the process cannot access this file because it is used by another process". Solving it will be probably a bit harder than expected, it should probably have its own issue and PR.

debris · 2018-10-01T13:03:16Z

ethcore/src/views/mod.rs

-        let _ = view!(HeaderView, &[]).parent_hash();
-    }
+	#[test]
+	#[should_panic]


why did you remove the panic message?

The windows panic message contains '' instead of '/' in the file path of the error message. Another option could be to have a copy of this tests for windows.

debris · 2018-10-01T13:03:51Z

rpc/src/tests/ws.rs

@@ -105,6 +105,7 @@ mod testing {
 		http_client::assert_security_headers_present(&response.headers, None);
 	}

+	#[cfg(not(target_os = "windows"))]


why is this test ignored? :)

I did not identify the root of the issue, the test is failing on a connection timeout.

debris · 2018-10-01T13:04:02Z

ethstore/src/accounts_dir/vault.rs

+		use std::path::Path;
+
+		assert_eq!(&make_vault_dir_path("/home/user/parity", "vault", true).unwrap(), &Path::new("/home/user/parity/vault"));
+		assert_eq!(&make_vault_dir_path("/home/user/parity", "*bad-name*", false).unwrap(), &Path::new("/home/user/parity/*bad-name*"));


debris · 2018-10-01T13:04:25Z

parity/configuration.rs

 		let std = parse(&["parity"]);
 		let base = parse(&["parity", "--base-path", "/test"]);

 		let base_path = ::dir::default_data_path();
 		let local_path = ::dir::default_local_path();
 		assert_eq!(std.directories().cache, dir::helpers::replace_home_and_local(&base_path, &local_path, ::dir::CACHE_PATH));
-		assert_eq!(base.directories().cache, "/test/cache");
+		assert_eq!(path::Path::new(&base.directories().cache), path::Path::new("/test/cache"));


5chdn · 2018-10-08T11:05:46Z

what's the status of this?

cheme · 2018-10-08T12:49:53Z

I think there is valid fixes for windows test that should be merged, there is also two skipped tests, so before merging a issue should be created (one is about directory deletion failing on windows and is not that trivial, the other one is not identified).
With this PR I could run test on a windows 10 VM, but the integration seems still broken : to see the issue in integration you need to change test.sh to run on one thread and you see a 'segfault' error. This segfault may be related to ci envt or related to #9702 .
For this PR I am in favor of merging it and creating another issue for the directory deletion test and maybe for the ci segfault in devops.

5chdn · 2018-10-26T11:43:13Z

Could you review this @ordian @ascjones ?

ordian

I'm in favor of merging this PR, but note, that:

I don't have a Windows machine atm to test this (but will by the end of this week, not sure if VM is representative enough).
Not sure whether it's worth the effort if we merge ci: remove failing tests for android, windows, and macos #9788.
It doesn't fix tests on our Windows CI, but seems like a reasonable step towards making it happen.

ascjones

Agree with @ordian. Changes LGTM but I was waiting to run it on my Windows machine, but that is back in London.

5chdn · 2018-10-31T10:50:34Z

Merging, it's Windows ;)

cheme added 2 commits September 25, 2018 18:30

Fixing and disabling some tests for windows 10 compatibility.

81a398c

Few adjustment for windows in tests (eg bigger timeout for keyserver …

41c0bb1

…tests)

NikVolf added A0-pleasereview 🤓 Pull request needs code review. M1-ci 🙉 Continuous integration. labels Sep 28, 2018

NikVolf reviewed Sep 28, 2018

View reviewed changes

5chdn added this to the 2.2 milestone Sep 29, 2018

Spaces and temporary single thread ci (to be able to spot the error).

8c45a55

cheme removed the A0-pleasereview 🤓 Pull request needs code review. label Oct 1, 2018

Merge branch 'master' into ec-winfix

a3c2c36

debris reviewed Oct 1, 2018

View reviewed changes

debris added the M4-core ⛓ Core client code / Rust. label Oct 1, 2018

5chdn added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Oct 1, 2018

Merge branch 'master' into ec-winfix

14a31bf

5chdn added A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Oct 8, 2018

5chdn added A0-pleasereview 🤓 Pull request needs code review. and removed A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. labels Oct 8, 2018

5chdn requested review from ascjones and ordian October 26, 2018 11:42

5chdn modified the milestones: 2.2, 2.3 Oct 29, 2018

ordian approved these changes Oct 29, 2018

View reviewed changes

ascjones approved these changes Oct 29, 2018

View reviewed changes

5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 31, 2018

5chdn merged commit 39f25d2 into master Oct 31, 2018

5chdn deleted the ec-winfix branch October 31, 2018 10:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests modification for windows CI #9671

Tests modification for windows CI #9671

cheme commented Sep 28, 2018 •

edited

Loading

parity-cla-bot commented Sep 28, 2018

NikVolf Sep 28, 2018

debris Oct 1, 2018

cheme Oct 1, 2018

debris Oct 1, 2018

debris Oct 1, 2018

cheme Oct 1, 2018

cheme Oct 1, 2018

debris Oct 1, 2018

cheme Oct 1, 2018

debris Oct 1, 2018

cheme Oct 1, 2018

debris Oct 1, 2018

debris Oct 1, 2018

5chdn commented Oct 8, 2018

cheme commented Oct 8, 2018 •

edited

Loading

5chdn commented Oct 26, 2018

ordian left a comment •

edited

Loading

ascjones left a comment

5chdn commented Oct 31, 2018

Tests modification for windows CI #9671

Tests modification for windows CI #9671

Conversation

cheme commented Sep 28, 2018 • edited Loading

parity-cla-bot commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

5chdn commented Oct 8, 2018

cheme commented Oct 8, 2018 • edited Loading

5chdn commented Oct 26, 2018

ordian left a comment • edited Loading

Choose a reason for hiding this comment

ascjones left a comment

Choose a reason for hiding this comment

5chdn commented Oct 31, 2018

cheme commented Sep 28, 2018 •

edited

Loading

cheme commented Oct 8, 2018 •

edited

Loading

ordian left a comment •

edited

Loading