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

Use custom test framework to cargo test ! #41

Merged

Conversation

ian-h-chamberlain
Copy link
Member

@ian-h-chamberlain ian-h-chamberlain commented Feb 3, 2022

Closes #25 , I think...

This basically plugs in the default test framework with hardcoded options
You can test it manually like this until cargo-3ds has support (.elf filename with hash may be different):

smdhtool --create ctru-rs-test "test ctru-s" "no author" /opt/devkitpro/libctru/default_icon.png target/armv6k-nintendo-3ds/debug/deps/ctru.smdh

cargo 3ds test  --no-run --lib --package ctru-rs && \
    3dsxtool --smdh=target/armv6k-nintendo-3ds/debug/deps/ctru.smdh \
        target/armv6k-nintendo-3ds/debug/deps/ctru-2d17fcbfba13d8a9.elf \
        target/armv6k-nintendo-3ds/debug/deps/ctru-test.3dsx && \
    3dslink  target/armv6k-nintendo-3ds/debug/deps/ctru-test.3dsx

For now I included the Ps changes since they seemed easy to plug into this, but I can take them out / wait for that to merge:

Things that seem to work correctly:

  • Threading
  • Output capture

Things that don't / won't work, as far as I know:

  • Subprocessing
  • Color output (we may be able to work around this)

Things I haven't tested

  • Shuffling tests
  • Benchmark tests
  • Filtering / ignoring tests
  • Timeouts
  • Streamed output while running (probably would require some changes to flush the gfx while the tests are running)
  • Logfile

Screenshots

Failed Test
image

All Passed
image

ctru-rs/src/test_runner.rs Outdated Show resolved Hide resolved
@Meziu
Copy link
Member

Meziu commented Feb 3, 2022

This basically plugs in the default test framework with hardcoded options
You can test it manually like this until cargo-3ds has support (.elf filename with hash may be different)

We should do that first. There is surely a way to get the target path of the test executable.

@ian-h-chamberlain
Copy link
Member Author

This basically plugs in the default test framework with hardcoded options
You can test it manually like this until cargo-3ds has support (.elf filename with hash may be different)

We should do that first. There is surely a way to get the target path of the test executable.

Ok, I think I've found the relevant details needed to make it work in cargo-3ds. I will make that my next priority after the Ps stuff is wrapped up.

ctru-rs/src/services/ps.rs Outdated Show resolved Hide resolved
ctru-rs/src/services/ps.rs Outdated Show resolved Hide resolved
ctru-rs/src/test_runner.rs Outdated Show resolved Hide resolved
ctru-rs/src/test_runner.rs Show resolved Hide resolved
ctru-rs/src/test_runner.rs Outdated Show resolved Hide resolved
ctru-rs/src/test_runner.rs Outdated Show resolved Hide resolved
ctru-rs/src/test_runner.rs Outdated Show resolved Hide resolved
ctru-rs/src/test_runner.rs Show resolved Hide resolved
@Meziu
Copy link
Member

Meziu commented Feb 6, 2022

We absolutely need this, and have to start putting a lot of tests in ctru. As we saw with ThreadLocals, we can't be sure of changes because they have a massive impact on the whole ecosystem.

@ian-h-chamberlain
Copy link
Member Author

Sorry for the lack of updates here, I was away for the weekend and didn't have any time to work on it. Hoping to get to update this PR to address concerns this week and try to get this merged.

- Switch to single-threaded test and fix thread-local hashmap issue.
- Use apt loop instead of `loop {}` , and wait for vblank.
- Remove extraneous pthread stub.
@ian-h-chamberlain
Copy link
Member Author

Pushed a set of changes based on feedback, I still need to do the cargo-3ds work, but it should at least build as-is with all the crates updated to the latest git versions.

})
.unwrap()
.join()
.expect_err("should have panicked");
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bit weird, hence the large comment, and relies on some implementation details of std, which I don't love. Here is the output I was referencing:

tests_Pass

Perhaps it makes more sense to remove this test case entirely, since we don't necessarily care if HashMap::new panics, but we do want the positive test case of being able to construct one when we have initialized PS. Let me know what you think or if you have a cleaner suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this will be fixed once we move to std threads: rust-lang/rust#78227

I think we should keep this test regardless.

ctru-rs/src/test_runner.rs Outdated Show resolved Hide resolved
ctru-rs/src/test_runner.rs Outdated Show resolved Hide resolved
Copy link
Member

@AzureMarker AzureMarker left a comment

Choose a reason for hiding this comment

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

I haven't gotten a chance to actually test this, but it can merge before I get to it.

@Meziu Meziu merged commit 56f4b33 into rust3ds:master Feb 8, 2022
@ian-h-chamberlain ian-h-chamberlain deleted the testing/custom-test-framework branch February 24, 2024 19:55
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.

Unit Testing
3 participants