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 IntegrationTest::app_dir_preprocessor #397

Merged
merged 2 commits into from
May 23, 2022

Conversation

Malax
Copy link
Member

@Malax Malax commented May 20, 2022

This allows users to have the app directory (test fixture) to be modified in a safe way, just before the
integration test runs.

Usage Example

This is a cutlass test ported to libcnb-test for heroku/maven. Original cutlass test can be found here: https://github.com/heroku/buildpacks-jvm/blob/e91747a4310d566a2ecf3b88ce94ed86744c6346/test/specs/maven/misc_spec.rb#L3-L13

#[test]
#[cfg(target_family = "unix")]
pub fn maven_wrapper_executable_bit() {
    IntegrationTest::new(BUILDER_NAME, "../../test/fixtures/simple-http-service")
        .buildpacks(vec![
            BuildpackReference::Other(String::from("heroku/jvm")),
            BuildpackReference::Crate,
        ])
        .app_dir_preprocessor(|app_dir| {
            use std::os::unix::fs::PermissionsExt;
            fs::set_permissions(app_dir.join("mvnw"), Permissions::from_mode(0o444)).unwrap();
        })
        .run_test(|context| {
            assert_contains!(context.pack_stdout, "Successfully built image");
        });
}

Closes GUS-W-11177468

This allows users to have the app directory (test fixture) to be modified in a safe way, just before the
integration test runs.
@Malax Malax marked this pull request as ready for review May 20, 2022 13:02
@Malax Malax requested a review from a team as a code owner May 20, 2022 13:02
@Malax Malax added libcnb-test enhancement New feature or request labels May 20, 2022
Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

In the future, perhaps we can switch to only performing the app_dir copy iff app_dir_preprocessor is set? We still expose app_dir during the test itself, and in the rustdocs for it say modifying is fine - but perhaps when we eventually support repeat builds, we could make users use app_dir_preprocessor for repeat builds too, at which point we can make app_dir non-public on IntegrationTestContext, and so it would be safe to not copy unless a pre-processor is set?

libcnb-test/CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com>
@Malax
Copy link
Member Author

Malax commented May 23, 2022

In the future, perhaps we can switch to only performing the app_dir copy iff app_dir_preprocessor is set? We still expose app_dir during the test itself, and in the rustdocs for it say modifying is fine - but perhaps when we eventually support repeat builds, we could make users use app_dir_preprocessor for repeat builds too, at which point we can make app_dir non-public on IntegrationTestContext, and so it would be safe to not copy unless a pre-processor is set?

In fact, this is broken out of a larger change that allows repeat builds. The idea is to allow app_dir_preprocessor for repeat builds as well. 👍🏻 It is still under heavy development, I'll consider removing app_dir from IntegrationTestContext and dropping app_dir copying when it's not required.

I assume your main concern is removing unnecessary IO during tests (the app_dir copy) for a slight speed-up?

@Malax Malax merged commit 6452900 into main May 23, 2022
@Malax Malax deleted the malax/libcnb-test-app-dir-preprocessor branch May 23, 2022 09:05
@edmorley
Copy link
Member

I assume your main concern is removing unnecessary IO during tests (the app_dir copy) for a slight speed-up?

Yeah

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request libcnb-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants