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

Improve integration test coverage #75

Merged
merged 3 commits into from
Jun 20, 2022
Merged

Conversation

edmorley
Copy link
Member

@edmorley edmorley commented Jun 17, 2022

Whilst the unit tests for this repository are very comprehensive, much of the end buildpack behaviour depends on Pack CLI / lifecycle / the buildpack API version, which aren't tested by the unit tests. For example Buildpack API 0.6 changed the default process handling in a breaking way.

Now that libcnb-test has added support for more methods of invoking the built containers (see heroku/libcnb.rs#366), we are able to add integration tests for more than just a single web process example.

This PR:

  • Adds five additional integration tests.
  • Drops the usage of ureq / making HTTPS requests to the web process, in favour of a simpler echo / logs output based approach. The only reason the former approach was used was because of the "valid Procfile but not valid YAML" case (see 741bbc0), however that is now tested via a simpler echo fixture that includes the offending "key: value" substring, rather than the socat example command.
  • Drops usage of tempfile / manually writing files in the unit tests, in favour of reading files from the fixtures directory.
  • Uses indoc! to assert against multi-line log output strings, to make the end output more clearly visible from the test, and also reduce the chance of false positives (for example, Procfile declares types -> web would previously successfully match against Procfile declares types -> web, something-else since there was no end of line marker).
  • Uses the new ignore = "reason" support in Rust 1.61, to make it clearer why (integration) tests are being skipped when running cargo test.
  • Remove the redundant .buildpacks(vec![BuildpackReference::Crate]) calls, since the current buildpack is the default when using libcnb-test's run_test().
  • Removes RUST_BACKTRACE=1 from the Circle CI configs, since in general the backtraces are noisy (making it harder to read the actual error) and add little value over the file+line number that's already output in the standard error message. In rare scenarios where the backtrace is actually useful, it can be enabled locally, or on a Circle CI run with a debugging commit pushed to the PR etc.
  • Adds heroku/builder:22 to the trusted builders list, since otherwise it slows down builds, and also means log output has additional prefixes, which mess up the multi-line assertions. Longer term we will add this builder to Pack's built-in trusted builders list, and also make libcnb-test pass --trust-builder to the pack build command:
    libcnb-test should pass --trust-builder to pack build libcnb.rs#407

GUS-W-11311966.

@edmorley edmorley self-assigned this Jun 17, 2022
@edmorley edmorley marked this pull request as ready for review June 17, 2022 14:20
@edmorley edmorley requested a review from a team as a code owner June 17, 2022 14:20
Now that libcnb-test has added support for more methods of invoking
the built containers (see heroku/libcnb.rs#366),
we are able to add integration tests for more than just the `web` process
handling. 

Whilst the unit tests for this repository are very comprehensive, much of
the end buildpack behaviour depends on pack/lifecycle/the buildpack API
version, which aren't tested by the unit tests. For example buildpack API 0.6
changed the default process handling in a breaking way.

This PR:
- Adds five additional integration tests.
- Drops the usage of `ureq` / making HTTPS requests to the `web` process,
  in favour of a simpler `echo` / logs output based approach. The only reason
  the former approach was used was because of the "valid Procfile but not valid
  YAML" case, however that is now tested via a simpler `echo` fixture that includes
  the offending `"key: value"` substring, rather than  the `socat` example command.
- Drops usage of `tempfile` / manually writing files in the unit tests, in favour of
  reading files from the fixtures directory.
- Uses `indoc!` to assert against multi-line log output strings, to make the end
  output more clearly visible from the test, and also reduce the chance of false
  positives (for example, `Procfile declares types -> web` would previously
  successfully match against `Procfile declares types -> web, something-else`
  since there was no end of line marker).
- Uses the new `ignore = "reason"` support in Rust 1.61, to make it
  clearer why tests are being skipped when running `cargo test`.
- Adds `heroku/builder:22` to the trusted builders list, since otherwise
  it slows down builds, and also means log output has additional prefixes,
  which mess up the multi-line assertions. Longer term we will add this
  builder to Pack's built-in trusted builders list, and make `libcnb-test`
  pass `--trust-builder` to the `pack build` command:
  heroku/libcnb.rs#407

GUS-W-11311966.
@edmorley edmorley force-pushed the edmorley/more-integration-tests branch from 3868cbb to 855661b Compare June 17, 2022 14:45
Copy link
Member

@Malax Malax left a comment

Choose a reason for hiding this comment

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

🙌🏻 This is great work!

edmorley added 2 commits June 20, 2022 13:32
Since testing the current buildpack is the default, so no need
to specify it explicitly.
It makes failures more noisy and harder to actually read the
most important part of the error messages. In general the
backtrace adds little value, particularly for our use-cases of
fairly simple program/test flow.
@edmorley edmorley merged commit 9a9ba97 into main Jun 20, 2022
@edmorley edmorley deleted the edmorley/more-integration-tests branch June 20, 2022 12:52
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.

2 participants