CONTRIBUTING.md and Testing changes

[bcheidemann]

The changes in this PR are based largely on those from #48 (which seems to be abandoned). I have also included additional changes to support testing the Node/WASM bindings using cargo. It's not perhaps the most elegant way to do it, but it at least works as a starting point.

[bcheidemann]

Since this is branched off from #85, I've marked it as a draft until this is merged. I'll then rebase this PR and mark it as being ready for review.

[bcheidemann]

@mpalmer this one's ready for review now

[mpalmer]

Thanks for the tidy up. As it's so extensive, it'll take me a while to do a full review, but rest assured, my eyes are on it.

[bcheidemann]

Thanks for the tidy up. As it's so extensive, it'll take me a while to do a full review, but rest assured, my eyes are on it.

No problem 🙂 TYT

[mpalmer]

Phew, finally found the time to finish going through this. A couple of docs typos are the "requested changes", and I've got a couple of questions, but overall this seems like a definite improvement on how testing is done now.

[mpalmer]

I've not set --nocapture by default on CI test runs before. What's the benefits to doing it?

[bcheidemann]

This was a bit of a hack and I'm happy to say I've pushed some commits to remove this now 🙂

Because we don't currently support all features on JS targets, we were "skipping" certain unsupported test cases on JS targets. It was possible to skip running a test on JS targets by adding "targets": { "node": false } to the test.json file. However, the default test harness doesn't support #[ignore]ing at runtime, so I implemented a workaround to conditionally skip running the test by simply returning early. But this misleadingly reported the test as passed, so this --nocapture was another workaround to allow "reporting" the test as skipped. The cargo test output looked something like this:

test snapshot::_003_successful_globs ... skipped
test snapshot::_003_successful_globs ... ok
test snapshot::_004_failing_globs ... skipped
test snapshot::_004_failing_globs ... ok
test snapshot::_004a_failing_negative_glob ... skipped
test snapshot::_004a_failing_negative_glob ... ok
test snapshot::_011_subdir_globs ... skipped
test snapshot::_011_subdir_globs ... ok
...
test result: ok. 12 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.79s
                                      ^^^^^^^^^ 🫤

Without --nocapture, the test snapshot::_* ... skipped lines would not have been logged.

Anyway, I wasn't happy with this solution as it's quite hacky and I was already experimenting with ways to conditionally #[ignore] tests in fixtures. I've now published a new version of fixtures which makes skipping tests based on cargo features a lot cleaner:

#[fixtures(["tests/fixtures/*"])]
#[cfg_attr(
    feature = "test-js",
    fixtures::ignore(
        paths = "tests/fixtures/003_successful_globs",
        reason = "Glob support is not implemented for JS targets yet."
    ),
    fixtures::ignore(
        paths = "tests/fixtures/004_failing_globs",
        reason = "Glob support is not implemented for JS targets yet."
    ),
    fixtures::ignore(
        paths = "tests/fixtures/004a_failing_negative_glob",
        reason = "Glob support is not implemented for JS targets yet."
    ),
    fixtures::ignore(
        paths = "tests/fixtures/011_subdir_globs",
        reason = "Glob support is not implemented for JS targets yet."
    )
)]
#[test]
fn snapshot(dir: &Path) {
    SnapshotTest::new(dir).execute();
}

Because this adds an actual #[ignore = "..."] attribute, the ignored test cases are reported correctly now:

test snapshot::_003_successful_globs ... ignored, Glob support is not implemented for JS targets yet.
test snapshot::_004_failing_globs ... ignored, Glob support is not implemented for JS targets yet.
test snapshot::_004a_failing_negative_glob ... ignored, Glob support is not implemented for JS targets yet.
test snapshot::_011_subdir_globs ... ignored, Glob support is not implemented for JS targets yet.
...
test result: ok. 8 passed; 0 failed; 4 ignored; 0 measured; 0 filtered out; finished in 0.79s
                                     ^^^^^^^^^ 🤗

This is implemented in 0e4ed5e, f1a9d25, and 76bb506.

In future, I plan to add support for the following syntax in fixtures. Sadly, I've more or less exhausted my supply of round tuits so this will need to wait for now 😛.

#[fixtures(["tests/fixtures/*"])]
#[cfg_attr(
    feature = "test-js",
    fixtures::ignore(
        paths = [
            "tests/fixtures/003_successful_globs",
            "tests/fixtures/004_failing_globs",
            "tests/fixtures/004a_failing_negative_glob",
            "tests/fixtures/011_subdir_globs",
        ],
        reason = "Glob support is not implemented for JS targets yet."
    )
)]
#[test]
fn snapshot(dir: &Path) {
    SnapshotTest::new(dir).execute();
}

[mpalmer]

I think I understand why this is necessary based on the current approach you've implemented, but I've gotten extra paranoid about build dependencies in recent times, mostly for supply-chain security reasons. Do you think that "locking" the version of fixtures used as a build dependency is a reasonable approach, or does that cause as many problems as it solves?

[bcheidemann]

I've gotten extra paranoid about build dependencies in recent times, mostly for supply-chain security reasons.

Yup, that's a pretty reasonable concern.

Do you think that "locking" the version of fixtures used as a build dependency is a reasonable approach, or does that cause as many problems as it solves?

Actually, fixtures::build::watch_dir("path/to/fixtures") is just a thin wrapper around println!("cargo:rerun-if-changed=path/to/fixtures"). It's only job is to ensure that changes to the fixtures are picked up (without it, cargo test would cache the previous output, so even if e.g. a new test was added, it wouldn't be run until there was some sort of code change).

I've pushed 0c157df which removes the build dependency entirely in favour of using println directly.

[bcheidemann]

@mpalmer thanks for the review! I have applied all of the suggestions, and pushed some commits with further improvements relating to your questions. Let me know if you'd like me to make any further changes 🙂

[mpalmer]

Random thought I just had: since this env var is capable of injecting arbitrary arguments into the wasm-pack call, is there any chance of someone being able to potentially trigger Shenanigans if they're able to influence the environment in which build-wasm.sh (or test-wasm.sh) is invoked? Not knowing anything about wasm-pack, it's possible that there's no way that a command line option to wasm-pack could possibly cause problems, so I'll leave the call up to you, it just struck me as a potentially risky way to inject args, compared to (say) passing it on the command line, or detecting the presence of an env var (eg WASM_PACK_NO_OPT) and setting just the option that's needed.

[bcheidemann]

Hey @mpalmer, that's a good question.

I don't think any of the wasm-pack arguments are directly exploitable, but wasm-pack passes flags to cargo build and some of those flags could be exploitable under certain circumstances. However, I don't think that's actually exploitable though, because of the way wasm-pack interprets its arguments.

For example:

WASM_PACK_BUILD_FLAGS='--no-opt --manifest-path Cargo2.toml' ./scripts/build-wasm.sh

Becomes:

npx wasm-pack build --no-opt --manifest-path Cargo2.toml --out-dir target/wasm-pack/build --no-typescript --target nodejs --no-default-features --features js
                    |______| |______________________________________________________________________________________________________________________________|
                    |         Passed to `cargo build`
                    | Consumed by `wasm-pack`

This fails with:

error: unexpected argument '--no-typescript' found

That said, even if this isn't exploitable, does that make it a good idea? What happens if wasm-pack or cargo build behaviors change? What happens if someone sees this pattern and adds CARGO_BUILD_FLAGS without thinking through the implications.

I pushed a commit to my fork which removes WASM_PACK_BUILD_FLAGS but GitHub is giving me a 500 error when I try to raise a PR...

[mpalmer]

This doesn't appear to consume WASM_PACK_BUILD_FLAGS, or is there some magicks deep in the build chain that does so? (That would, I guess, explain why that env var is being used in the first place...) 🤷

[bcheidemann]

Ah, test-wasm.sh requires build-wasm.sh to have been run already. It doesn't automatically rebuild the WASM binary, so there's no need to accept WASM_PACK_BUILD_FLAGS. I'd be happy to update it to automatically build the WASM binary before running the tests. That would more closely mirror the behavior of the native tests, so it's probably more intuitive.