Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

test(rome_cli): add snapshots for file system and console session #3035

Merged
merged 3 commits into from
Aug 9, 2022

Conversation

ematipico
Copy link
Contributor

Summary

This PR improves the CLI tests by creating a snapshot. The snapshot will contain:

  • the content of the configuration file, if it exists
  • the files contained inside the memory file system during the test run of a single test (excluded the configuration file)
  • the messages printed by the console, if any (excluded the timing messages)

The reason why I created snapshots it's because I found myself at struggle sometimes, when I needed to test specific cases. Also, there have been few issue before where some messages where not supposed to be there, and I couldn't catch them early. The snapshots, somehow, might help the testing experience a bit.

The snapshot have been added to some existing cases, but not all of them. I had to exclude some runs because of some issue that I couldn't debug (some test stalls for some reason).

Test Plan

The CI should not fail

@ematipico ematipico requested a review from leops as a code owner August 9, 2022 08:08
@ematipico ematipico requested a review from a team August 9, 2022 08:08
@ematipico ematipico temporarily deployed to aws August 9, 2022 08:08 Inactive
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Aug 9, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1813c79
Status: ✅  Deploy successful!
Preview URL: https://856ea47f.tools-8rn.pages.dev
Branch Preview URL: https://chore-snapshot-cli-tests.tools-8rn.pages.dev

View logs

@ematipico ematipico temporarily deployed to aws August 9, 2022 08:12 Inactive
@github-actions
Copy link

github-actions bot commented Aug 9, 2022

@ematipico ematipico temporarily deployed to aws August 9, 2022 08:28 Inactive
@leops
Copy link
Contributor

leops commented Aug 9, 2022

I wonder if we could transition the CLI test suite to be fully fixture and snapshot based: each test is a directory containing an fs directory with the initial state of the memory filesystem, an stdin file containing the command line to be run and eventually the console inputs for interactive commands, and the snapshot file with the modified files and console output

@ematipico
Copy link
Contributor Author

I wonder if we could transition the CLI test suite to be fully fixture and snapshot based: each test is a directory containing an fs directory with the initial state of the memory filesystem, an stdin file containing the command line to be run and eventually the console inputs for interactive commands, and the snapshot file with the modified files and console output

I really like the idea, I think this could work. But I would like to apply this proposal in another PR, because your will make a lot changes. If you're OK with the structure of the snapshots, I think we can merge it?

Copy link
Contributor

@leops leops left a comment

Choose a reason for hiding this comment

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

I'm still a bit concerned about why the upgrade_severity test would suddenly start hanging, but I couldn't reproduce the issue locally (I tried to run the test with and without a debugger attached but the issue never manifested). Otherwise the format of the snapshots themselves seems alright to me

if !self.messages.is_empty() {
content.push_str("## Messages\n\n");

for message in &self.messages {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: wrap the messages in a code block to have them rendered in a <pre> block in markdown previews and respect the line breaks

@ematipico ematipico temporarily deployed to aws August 9, 2022 13:27 Inactive
@ematipico ematipico temporarily deployed to aws August 9, 2022 13:33 Inactive
@ematipico ematipico temporarily deployed to aws August 9, 2022 13:34 Inactive
@ematipico ematipico merged commit cd6fbab into main Aug 9, 2022
@ematipico ematipico deleted the chore/snapshot-cli-tests branch August 9, 2022 13:51
IWANABETHATGUY pushed a commit to IWANABETHATGUY/tools that referenced this pull request Aug 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants