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

Roc test can now support multiple files #6967

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jfkonecn
Copy link
Contributor

@jfkonecn jfkonecn commented Aug 4, 2024

Closes #5103

I didn't do any fancy. I just loops through all the passed file. Let me know if you want me to do something else.

@smores56
Copy link
Collaborator

smores56 commented Aug 4, 2024

Seems that you have a clippy error. Can you fix this and run cargo clippy --workspace --tests -- --deny warnings before committing to validate you've fixed the issue?

     Checking roc_cli v0.0.1 (/Users/m1ci/actions-runner2/_work/roc/roc/crates/cli)
error: this `else { if .. }` block can be collapsed
   --> crates/cli/src/lib.rs:641:16
    |
641 |           } else {
    |  ________________^
642 | |             if matches.get_flag(FLAG_VERBOSE) {
643 | |                 println!("Compiled in {} ms.", compilation_duration.as_millis());
644 | |                 for module_test_results in results_by_module {
...   |
651 | |             }
652 | |         }
    | |_________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_else_if
    = note: `-D clippy::collapsible-else-if` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::collapsible_else_if)]`

Also, please sign your commits, a guide for which can be found in our CONTRIBUTING.md document.

@jfkonecn
Copy link
Contributor Author

jfkonecn commented Aug 5, 2024

I need to read the docs! 😆

@jfkonecn jfkonecn marked this pull request as draft August 6, 2024 12:56
@jfkonecn
Copy link
Contributor Author

jfkonecn commented Aug 6, 2024

Debugging time!

@jfkonecn jfkonecn marked this pull request as ready for review August 7, 2024 11:27
@jfkonecn
Copy link
Contributor Author

jfkonecn commented Aug 7, 2024

I'm not quite sure what was wrong with the pipeline. The test passing on my machine. I just did a rebase with the latest commit. I'm hoping that fixes it.

@jfkonecn jfkonecn force-pushed the test-mulitple-files branch 2 times, most recently from c3d684e to 78afd36 Compare August 11, 2024 13:17
Copy link
Collaborator

@lukewilliamboswell lukewilliamboswell left a comment

Choose a reason for hiding this comment

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

Thank you for this @jfkonecn, it's looking good. I played with this a bit and found a few things that were unexpected. Would you mind looking into these?

I tried to run this on a folder but got an error. I was expecting this behaviour

if you specify a folder, it's like running roc test on each .roc file in that folder

$ cargo run -- test examples/
── FILE PROBLEM in examples/ ───────────────────────────────────────────────────

I tried to read this file:

    examples/

But ran into:

    is a directory

I ran agains multiple files, it prints out No expectations were found. multiple times. I was expecting this to just print once.

$ cargo run -- test examples/platform-switching/*.roc
Running `target/debug/roc test examples/platform-switching/main.roc examples/platform-switching/rocLovesC.roc examples/platform-switching/rocLovesRust.roc examples/platform-switching/rocLovesWebAssembly.roc examples/platform-switching/rocLovesZig.roc`
No expectations were found.
No expectations were found.
No expectations were found.
No expectations were found.
No expectations were found.
No expectations were found.

When I specific only two files, it printed out three times.

$ cargo run -- test examples/platform-switching/rocLovesZig.roc examples/platform-switching/rocLovesRust.roc
    Finished dev [unoptimized + debuginfo] target(s) in 0.41s
     Running `target/debug/roc test examples/platform-switching/rocLovesZig.roc examples/platform-switching/rocLovesRust.roc`
No expectations were found.
No expectations were found.
No expectations were found.

@@ -498,18 +499,20 @@ pub fn test(matches: &ArgMatches, target: Target) -> io::Result<i32> {
Some(n) => Threading::AtMost(*n),
};

let path = matches.get_one::<PathBuf>(ROC_FILE).unwrap();
// let path = matches.get_one::<PathBuf>(ROC_FILE).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this left here by mistake?

@jfkonecn
Copy link
Contributor Author

Thank you for this @jfkonecn, it's looking good. I played with this a bit and found a few things that were unexpected. Would you mind looking into these?

I tried to run this on a folder but got an error. I was expecting this behaviour

if you specify a folder, it's like running roc test on each .roc file in that folder


$ cargo run -- test examples/

── FILE PROBLEM in examples/ ───────────────────────────────────────────────────



I tried to read this file:



    examples/



But ran into:



    is a directory

I ran agains multiple files, it prints out No expectations were found. multiple times. I was expecting this to just print once.


$ cargo run -- test examples/platform-switching/*.roc

Running `target/debug/roc test examples/platform-switching/main.roc examples/platform-switching/rocLovesC.roc examples/platform-switching/rocLovesRust.roc examples/platform-switching/rocLovesWebAssembly.roc examples/platform-switching/rocLovesZig.roc`

No expectations were found.

No expectations were found.

No expectations were found.

No expectations were found.

No expectations were found.

No expectations were found.

When I specific only two files, it printed out three times.


$ cargo run -- test examples/platform-switching/rocLovesZig.roc examples/platform-switching/rocLovesRust.roc

    Finished dev [unoptimized + debuginfo] target(s) in 0.41s

     Running `target/debug/roc test examples/platform-switching/rocLovesZig.roc examples/platform-switching/rocLovesRust.roc`

No expectations were found.

No expectations were found.

No expectations were found.

Thanks for the review! I'll take a look.

@jfkonecn jfkonecn marked this pull request as draft September 1, 2024 18:36
@jfkonecn jfkonecn force-pushed the test-mulitple-files branch 3 times, most recently from 32c5ca3 to bd1b7c3 Compare September 2, 2024 14:46
@jfkonecn
Copy link
Contributor Author

jfkonecn commented Sep 2, 2024

@lukewilliamboswell I've addressed you comments.

I'm happy to do any code clean up you want me to do even if you think it's a minor issue.

@jfkonecn jfkonecn marked this pull request as ready for review September 2, 2024 14:49
@lukewilliamboswell
Copy link
Collaborator

Thank you @jfkonecn

When I run it against the examples/ directory it panics with the following.

$ nix develop
$ cargo run -- test examples/
thread '<unnamed>' panicked at crates/compiler/solve/src/to_var.rs:507:17:
assertion failed: !tags.is_empty() || !ext_slice.is_empty()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@jfkonecn
Copy link
Contributor Author

jfkonecn commented Sep 3, 2024

Alright more debugging! 😆

@lukewilliamboswell
Copy link
Collaborator

@agu-z -- would you be able to look at this? I'm interested to know if we should log a separate issue and merge this PR as is?

Below is the assertion that is being hit. This seems unrelated to the changes in this PR.

// An empty tags is inefficient (but would be correct)
// If hit, try to turn the value into an EmptyTagUnion in canonicalization
debug_assert!(!tags.is_empty() || !ext_slice.is_empty());

@jfkonecn jfkonecn marked this pull request as draft September 20, 2024 15:45
@jfkonecn
Copy link
Contributor Author

@lukewilliamboswell on the latest commit of main does your code generate the same error if you run cargo run -- test examples/ruby-interop/platform/main.roc? That seems to be the issue for me.

@agu-z
Copy link
Collaborator

agu-z commented Oct 26, 2024

@lukewilliamboswell I'm sorry I hadn't seen this. Do you suspect this is related to one my changes?

@agu-z
Copy link
Collaborator

agu-z commented Nov 5, 2024

@lukewilliamboswell @jfkonecn You can reproduce this on main by running roc test against the examples/virtual-dom-wip platform file:

roc test examples/virtual-dom-wip/platform/server-side.roc

When you run roc test against examples/ this file is included so you run into that issue. I think we should unblock this PR and test only against the files in examples/ that don't already fail on main.

There seems to be a mono test that reproduces this issue already (#6947), so I don't think we need to create a separate one.

@jfkonecn jfkonecn marked this pull request as ready for review November 5, 2024 11:45
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.

Update roc test to support multiple files
4 participants