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

Disable rexpect tests on Windows #478

Merged
merged 4 commits into from
Mar 16, 2018
Merged

Conversation

Marwes
Copy link
Member

@Marwes Marwes commented Mar 15, 2018

It seems to have been because of a race due to not waiting for output after the let io = ... command.

cc @memoryruins

Previously I didn't figure there was a point since gluon does not do
anything platform dependent but to be safe the tests should still run
there (noticed because rexpect only works on unix)
@memoryruins
Copy link
Contributor

Gooood point. It appears that the spawn_shell module of rexpect has blocking/wait features built-in for exactly this purpose. The codeblock below passes on my machine, but as we saw before, that isn't much of a metric for this test haha. If this helps though, I'll convert the rest of the tests to using execute to prevent any chance of flakiness elsewhere.

use rexpect::{spawn_bash};

..

let mut repl = spawn_bash(Some(TIMEOUT))?;
repl.execute(COMMAND, PROMPT)?;
repl.execute("let io = import! std.io", 
             "\\{[^}]+\\}")?;
repl.execute("io.println \"Hello world\"",
             "Hello world")?;

@Marwes
Copy link
Member Author

Marwes commented Mar 15, 2018

Gooood point. It appears that the spawn_shell module of rexpect has blocking/wait features built-in for exactly this purpose. The codeblock below passes on my machine, but as we saw before, that isn't much of a metric for this test haha. If this helps though, I'll convert the rest of the tests to using execute to prevent any chance of flakiness elsewhere.

Great! Though I think you don't need to convert all the tests, only the places which does not wait explicitly need changing. (That is, if a test already check using exp_* for the complete line after using send then it should be fine).

The change I did was unfortunately not enough since it happens to match on part of the output. (It appeared to be after running it successfully ~7 times in a row.) If it would match on the matching close brace then it would be enough.

@memoryruins
Copy link
Contributor

Ah true, and in that case we wouldn't need to switch to execute, as if the regex matches, exp_regex should be all thats needed. Interestingly the array import test hasn't had an issue yet.

It appears that "load_script" is unique in the output and is near the closing brace, and it has run 20 times successfully so far.

repl.execute("let io = import! std.io", 
             "load_script")?;

Maybe this plays into #467, as we don't need to show the entire script's source upon importing.
In python's repl, it imports silently and proceeds to the next prompt. If the module doesn't exist, it has a consistent error message (which we could test on too).

To save time and avoid guessing the flakiness, we could set the hello_world test back to ignore until changing how imports are displayed.

@Marwes
Copy link
Member Author

Marwes commented Mar 15, 2018

Quick and dirty solution could be to print out some unique string and wait for that.

// after the `let io = `
send("\"my long string\"");
exp_string("\"my long string\"");

?

@Marwes
Copy link
Member Author

Marwes commented Mar 15, 2018

It is rather curious that it happens so frequently on the 32-bit build.

@memoryruins
Copy link
Contributor

memoryruins commented Mar 15, 2018

Right? Same arch each time on travis.

Pulled the test into its own script to test variations a bit more quickly (using pbr for progress bar). Matching the panics on my x86_64 linux laptop after hundreds of loops, even with the unique string. Edit: trying with both the regex and the unique string again with 10k loops, currently on loop 3500 and hasn't panicked again yet..

let count = 1000;
let mut pb = ProgressBar::new(count);
pb.format("╢▌▌░╟");
    
for _i in 1..count {
    pb.inc();    
    || -> Result<()> { 
        // variations
        Ok(())
    }().unwrap_or_else(|err| panic!("{}", err));
}

pb.finish_print("success");

@Marwes Marwes changed the title Fix flakiness in the hello_world repl tests Disable rexpect tests on Windows Mar 15, 2018
@Marwes
Copy link
Member Author

Marwes commented Mar 15, 2018

@memoryruins So this didn't end up fixing the issue but I should still merge this for the windows fixes. I can wait with that though if it would cause a merge conflict though.

Does that mean you got a reproduction without using gluon's repl? Might be good to open an issue at https://github.com/philippkeller/rexpect/issues then?

@memoryruins
Copy link
Contributor

It won't create any conflicts, go ahead! The reproduction was while testing gluon's repl, except outside of cargo's tests. Needed an easier way to stress/mass test it.

repl.send_line("let io = import! std.io")?;
repl.exp_regex("\\{[^}]+\\}")?;

repl.send_line("\"UNIQUE\"")?;
repl.exp_string("GcStr(\"UNIQUE\")")?;

repl.send_line("io.println \"Hello world\"")?;
repl.exp_string("Hello world")?;

It's worth noting that this has now so far succeeded 5000+ loops (still going). If it does fail again, it may be worth opening an issue. We are some of the first to really give it a stress test like this.

@memoryruins
Copy link
Contributor

It failed on loop 5612 with a similar error as before. Opening an issue, they may catch something obvious in how it's tested with their api.

@Marwes Marwes merged commit b9ec896 into gluon-lang:master Mar 16, 2018
@Marwes Marwes deleted the repl_tests branch March 16, 2018 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants