Test suite cleanup #13148
Merged
Test suite cleanup #13148
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As inverted grep is something we do rather often, we add a convenience function to create an inverted grep gadget.
The compiledPlugins map with a mutex was essentially a glorified sync.Map, and therefore did not need to be defined as such. Instead, this commit replaces its uses by a normal sync.Map, and keeps it in the base test suite.
The interface for building a plugin through the test suite was confusing, as it would build the plugin and return its path, cache the path for the version built, and return the path regardless if something was built or not. While in the current state this is harmless as builds are idempotent, since the state of the plugin package/module does not change, this will in the future as we introduce customisation techniques on the plugin's directory and files, making this double-use potentially dangerous. Furthermore, the current behaviour is unclear, as the function hides that caching mechanism, which could come as a surprise for users attempting to build a plugin for the duration of a test, while the built plugin is linked to the test suite being run, and not the unit test being evaluated. Therefore this commit changes the sequence in which plugins are built and used. Now the `CompilePlugin` function builds a plugin, and does not return its path anymore, instead terminating the tests immediately if they fail. In normal test usage, a new `GetPluginPath` function is introduced, which looks-up the path in the suite's cache, failing immediately if invoked before the plugin is built. With this change, it is heavily advised to build plugins when initialising the suite, then in the tests, the GetPluginPath function should be used to get a plugin's path for interacting with packer commands.
The function's name was the same as the plugins test suite runner/init function, making it impossible to differentiate between the two when looking at the logs, or when attempting to filter which tests to run using the suite function's name.
lbajolet-hashicorp
added
the
tech-debt
The lib name for the common components for writing packer_test suites was not clear, and did not follow the convention established in Packer core and plugins. Therefore this commit does two things: first the lib is renamed into common as to follow this convention, and clearly document which components are common to all tests. Also checkers are placed in a subpackage of common, common/check, so that it is clearer what is meant to be used as checks for a command's execution status after it's been run, as part of Assert.
When a command asserts its output with checkers, by default it will register errors through a t.Errorf. While this works, in some cases we would want to stop execution immediately if a function's Assert fails, as the rest of the test may depend on the assertion being valid. In the current state, this means either getting the result of the run to check if an error was returned (not fully reliable as if the command was run multiple times, and the last run succeeded, we won't get an error), or relying on t.IsFailed() (completely reliable). Instead, we introduce a new function on packerCommand, that lets users change how Assert behaves, so that if an error was reported, instead of logging the error and flagging the test as failed, we can use t.Fatalf, so that the test immedately fails and stops execution.
When using a PackerCommand, the Run function was made public as a way to access the contents of an execution. This was clumsy as it had too many responsabilities, and was not needed strictly as Assert was performing the executions, as many times as required. This could introduce cases in which one run as spent by the caller, then the remainder were executed through Assert. Therefore, we change this convention. Now, run is private to the type, and only through Assert can a command be executed. If a test needs access to a command's output, stderr, or error, it can do so through the Output function, which requires Assert to be called first.
lbajolet-hashicorp
force-pushed
the
test_suite_cleanup
branch
from
e6ea0ba
to
e91558f
Compare
nywilken
approved these changes
Sep 9, 2024
There was a problem hiding this comment.
@lbajolet-hashicorp I believe we discussed this PR already and I approved verbally. I'm approving now. But please let me know if I missed anything new.
|
Nothing new here, I'll merge this now, thanks for the update @nywilken ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR aims to clarify usage of the packer black box test suites by renaming a couple of functions and entities, and reduce the responsabilities of some functions (BuildSimplePlugin/CompilePlugin), in order to make usage of the packer test suite simpler and clearer.