feat: add self replacer integration tests by alvarocabanas · Pull Request #2401 · newrelic/newrelic-agent-control

alvarocabanas · 2026-04-09T14:56:41Z

Summary

Adds comprehensive integration tests for the self-replacer crate that compile and run real binaries to verify self-replacement behavior works correctly on both Unix and Windows platforms.

Test Coverage:

Common tests (Unix + Windows):

✅ Full self-replacement workflow with version verification
✅ Rollback on invalid path
✅ Backup file creation with correct content

Unix-specific tests:

✅ Permission preservation during replacement

Uses Cargo's examples/ directory to provide a pre-built test binary that:

Reports its own content hash for verification
Performs self-replacement when invoked with --replace flag
Eliminates runtime compilation for ~20x faster tests (21s → 1s)

Structure

self-replacer/
├── examples/
│ └── self_replacing_binary.rs # Pre-built example binary
└── tests/
├── integration_tests.rs # All integration tests
└── test_helpers.rs # Helper functions (copy/modify binary)

DavSanchez

Great! Some comments.

self-replacer/tests/integration_tests.rs

self-replacer/tests/test_helpers.rs

self-replacer/tests/integration_tests.rs

self-replacer/tests/test_helpers.rs

DavSanchez

Looking great to start driving this functionality. Thanks!

self-replacer/tests/integration_tests.rs

DavSanchez · 2026-04-10T08:37:20Z

self-replacer/tests/integration_tests.rs

+
+        // Set specific permissions on v1
+        let mut perms = fs::metadata(&binary_v1).unwrap().permissions();
+        perms.set_mode(0o754);


Would be nice to have a constant or let binding for this one so it's easier to use down in the code, compare it and document what permission it is.

Done in last commit.

self-replacer/tests/test_helpers.rs

self-replacer/tests/integration_tests.rs

self-replacer/tests/test_helpers.rs

self-replacer/examples/self_replacing_binary.rs

sigilioso

Looks good! Find some additional questions below

sigilioso · 2026-04-13T09:22:25Z

self-replacer/Cargo.toml

+[[example]]
+name = "self_replacing_binary"
+test = false


Nice! Could we add a comment explaining why test = false is needed?

Maybe some comments regarding the purpose of the example (either here or in the example's rust file) would be useful as well.

Done in last commit.

sigilioso · 2026-04-13T09:32:21Z

self-replacer/tests/integration_tests.rs

+        backup_path
+    );
+
+    // Verify backup has the original hash


Nit:

Suggested change

// Verify backup has the original hash

// Verify that the binary in the backup is the original

Done in last commit

sigilioso · 2026-04-13T09:44:38Z

self-replacer/tests/test_helpers.rs

+/// This ensures tests work both locally and in CI without requiring a separate build step.
+pub fn get_example_binary() -> PathBuf {


Doesn't assert_cmd already take care of binaries in the examples path?

// assert_cmd looks in target/debug/examples/ automatically Command::cargo_bin("self_replacing_binary") .expect("Example binary 'self_replacing_binary' should exist");

I think that assert_cmd already calls cargo under the hood. Maybe we don't need this helper.

assert_cmd doesn't work with examples and would need to have this binary in the [[bin]] that is not optimal

DavSanchez · 2026-04-13T12:27:40Z

self-replacer/tests/integration_tests.rs

+    Command::new(&binary_path)
+        .assert()
+        .success()
+        .stdout(predicate::str::starts_with("HASH:"));
+
+    let output2 = Command::new(&binary_path)
+        .output()
+        .expect("Failed to get new hash");


This test runs the command two times each. Can't we retrieve the hash and assert on the assert_cmd::Command in the same run? I see there's get_output() that we can call on the Asserts that get generated on calls to assert().

Done in last commit

alvarocabanas requested a review from a team as a code owner April 9, 2026 14:56

alvarocabanas force-pushed the feat/NR_537430_self_replacement_testing_harness branch 7 times, most recently from 59c162c to 5a86f7b Compare April 9, 2026 16:30

DavSanchez reviewed Apr 9, 2026

View reviewed changes

self-replacer/tests/integration_tests.rs Outdated Show resolved Hide resolved

self-replacer/tests/test_helpers.rs Outdated Show resolved Hide resolved

self-replacer/tests/test_helpers.rs Outdated Show resolved Hide resolved

alvarocabanas force-pushed the feat/NR_537430_self_replacement_testing_harness branch 2 times, most recently from b15ed85 to fa91684 Compare April 10, 2026 07:14

alvarocabanas requested a review from DavSanchez April 10, 2026 07:14

sigilioso reviewed Apr 10, 2026

View reviewed changes

self-replacer/tests/integration_tests.rs Show resolved Hide resolved

sigilioso reviewed Apr 10, 2026

View reviewed changes

self-replacer/tests/test_helpers.rs Outdated Show resolved Hide resolved

sigilioso reviewed Apr 10, 2026

View reviewed changes

self-replacer/tests/test_helpers.rs Outdated Show resolved Hide resolved

DavSanchez previously approved these changes Apr 10, 2026

View reviewed changes

sigilioso reviewed Apr 10, 2026

View reviewed changes

self-replacer/tests/test_helpers.rs Outdated Show resolved Hide resolved

sigilioso reviewed Apr 10, 2026

View reviewed changes

self-replacer/examples/self_replacing_binary.rs Show resolved Hide resolved

alvarocabanas dismissed DavSanchez’s stale review via d66ca54 April 10, 2026 13:02

alvarocabanas requested review from DavSanchez and sigilioso April 10, 2026 13:20

alvarocabanas added 6 commits April 13, 2026 10:51

feat: add self replacer integration tests

6f16557

feat: use assert_cmd for self-replace integration tests.

1dafb45

feat: use cargo examples for the test binary.

d62f9de

feat: fmt.

8703070

feat: Condition permissions only for unix.

0631a52

feat: compile the example binary if it doesnt't exist

51e0d3a

alvarocabanas force-pushed the feat/NR_537430_self_replacement_testing_harness branch from e2ae2a0 to 51e0d3a Compare April 13, 2026 08:51

sigilioso reviewed Apr 13, 2026

View reviewed changes

feat: improve example binary comments

a1e9f73

alvarocabanas requested a review from sigilioso April 13, 2026 10:22

DavSanchez reviewed Apr 13, 2026

View reviewed changes

feat: improve assert command calls

aeb4f31

	// Verify backup has the original hash
	// Verify that the binary in the backup is the original

		/// This ensures tests work both locally and in CI without requiring a separate build step.
		pub fn get_example_binary() -> PathBuf {

Conversation

alvarocabanas commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DavSanchez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DavSanchez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sigilioso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alvarocabanas commented Apr 9, 2026 •

edited

Loading