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

Add tests and highlight issues with should_panic #683

Merged
merged 2 commits into from
Oct 27, 2024

Conversation

alfiedotwtf
Copy link
Contributor

@alfiedotwtf alfiedotwtf commented Oct 24, 2024

This PR contains tests that demonstrate inconsistencies found in #654.

Once this PR is merged, the following PRs can then be updated along with removing corresponding should_panic test attributes:

Since #668 builds on top of the fixes in #666 and #667 and so will have conflicts and duplication, it will need to wait for the above PRs to be merged first before being mergeable.

@alfiedotwtf alfiedotwtf requested a review from a team October 24, 2024 08:29
@alfiedotwtf alfiedotwtf self-assigned this Oct 24, 2024
@fuel-service-user
Copy link
Contributor

fuel-service-user commented Oct 24, 2024

LCOV of commit d66011f during CI #2074

Summary coverage rate:
  lines......: 86.3% (2466 of 2856 lines)
  functions..: 46.1% (380 of 825 functions)
  branches...: 63.1% (289 of 458 branches)

Files changed coverage rate: n/a

@JoshuaBatty
Copy link
Member

Nice work. Just wondering if we could look at adding some helper functions to cut down on code duplication.

Something like

fn verify_toolchain_executables(cfg: &TestConfig, component: &Component, toolchain: &Toolchain) {
    let toolchain_bin_dir = cfg.toolchain_bin_dir(toolchain.name.as_str());
    for executable in &component.executables {
        assert!(
            toolchain_bin_dir.join(executable).exists(),
            "Executable not found: {}",
            executable
        );
    }
}

Probably a few places code duplication could be removed. Looks good otherwise.

@alfiedotwtf alfiedotwtf marked this pull request as draft October 25, 2024 10:54
@alfiedotwtf alfiedotwtf marked this pull request as ready for review October 25, 2024 15:16
component/src/lib.rs Outdated Show resolved Hide resolved
@JoshuaBatty JoshuaBatty requested a review from a team October 25, 2024 21:51
tests/testcfg/mod.rs Outdated Show resolved Hide resolved
@alfiedotwtf alfiedotwtf requested review from JoshuaBatty and a team October 26, 2024 10:09
@kayagokalp
Copy link
Member

It might be better to remove (#654) from titles of the PRs before merging otherwise they will show up as:

Add tests and highlight issues with should_panic (#654) (#683) in the history which might a little confusing as none of the other commits are in that form. Otherwise looks good to me!

@alfiedotwtf alfiedotwtf changed the title Add tests and highlight issues with should_panic (#654) Add tests and highlight issues with should_panic Oct 27, 2024
@alfiedotwtf alfiedotwtf merged commit 51155e7 into master Oct 27, 2024
17 checks passed
@alfiedotwtf alfiedotwtf deleted the alfie/tests-should-panic branch October 27, 2024 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants