Skip to content

Harder, Better, Faster, Stronger CLI integration tests#476

Merged
javiertoledo merged 8 commits intomasterfrom
fix/integration-tests
Nov 3, 2020
Merged

Harder, Better, Faster, Stronger CLI integration tests#476
javiertoledo merged 8 commits intomasterfrom
fix/integration-tests

Conversation

@javiertoledo
Copy link
Member

Description

We have been experiencing reliability issues with CLI integration tests. They were directly changing the files from the sample project in the package framework-integration-tests, introducing issues that then affected deployment tests and others, as they all rely on these files to be unaltered.

You may want to listen to the OST of this PR while reviewing it 😎

Changes

In this PR, I've reworked the CLI integration tests with the hopes of solving the mentioned reliability issues and make the tests faster the process. To do that I've done the following:

  • Removed file management from setup.ts (Tests should be self-contained)
  • Each test file creates a sandbox folder where they can mess up the file system in any way they want without altering future tests. For commands that require an existing project, there's a new createSandboxProject function that creates a new copy of the sample project in the integration tests package.
  • Replaced exec calls with the promise-wrapped version from package child-process-promise for consistency and code simplicity.
  • Created a loadFixture helper and moved calls to low-level fs functions to fileHelper.ts to make tests code more readable
  • Replaced exact string comparations to matchers in many expect blocks to make them a bit more resilient to changes.
  • prompt interaction in project generator tests now waits for the actual prompt instead of writing the next thing every second.
  • Lots of changes to make the coder easier to understand by humans (esp. regexes)
  • Bonus: Added a VSCode launch descriptor to ease debugging CLI integration tests.
  • Bonus: Fixed the CLI config to keep a consistent context for skipGit and skipInstall flags (functionally there's no difference)

Checks

  • Project Builds
  • Project passes tests and checks
    -Updated documentation accordingly (not needed)

Additional notes

This change doesn't guarantee that other integration tests are reliable, I'll tackle that in future PRs.

@javiertoledo javiertoledo self-assigned this Oct 25, 2020
@javiertoledo javiertoledo added feature-request New feature or request hacktoberfest-accepted size: L Tasks that imply many files. labels Oct 25, 2020
@javiertoledo javiertoledo force-pushed the fix/integration-tests branch from 05438fe to 95cd20e Compare October 25, 2020 12:38
Copy link

@NickSeagull NickSeagull left a comment

Choose a reason for hiding this comment

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

LGTM!


export const removeFolders = (paths: Array<string>): Array<Promise<void>> =>
paths.map((path: string) => rmdir(path, { recursive: true }))
export const createFolder = (folder: string): void => {

Choose a reason for hiding this comment

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

Maybe it'd be better to call this ensureFolderExists? As it doesn't create it if it exists

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense...

@javiertoledo
Copy link
Member Author

Ooops, AWS tests failing, I'll investigate...

Copy link
Contributor

@charlietfe charlietfe left a comment

Choose a reason for hiding this comment

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

Nothing to add, much cleaner and easy to understand tests. I did a nitty picky comment that you can omit it if you want.

So let's 🚢 🇮🇹 dude!


describe('Command', () => {
const cliPath = path.join('..', 'cli', 'bin', 'run')
const cliPath = path.join('..', '..', 'cli', 'bin', 'run')
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit-picky, it looks like the path segments never change in cli tests. Does it make sense to use a placeholder constant with the segments and add that const to the fileHelper?

it('should create a new command', async () => {
const expectedOutputRegex = new RegExp(
/(.+) boost (.+)?new:command(.+)? (.+)\n- Verifying project\n(.+) Verifying project\n- Creating new command\n(.+) Creating new command\n(.+) Command generated!\n/
['boost new:command', 'Verifying project', 'Creating new command', 'Command generated'].join('(.|\n)*')
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh god, what we had here, clearness to the rescue!

Copy link
Contributor

@adriangmweb adriangmweb left a comment

Choose a reason for hiding this comment

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

It looks great! Much cleaner and easier to understand now.

Integrations tests are failing when trying to create a scheduled command, but anyway LGTM! 🚀

Javier Toledo added 3 commits November 3, 2020 20:41
- Moved it to the AWS provider integration tests
- Introduced a specific event
- Used the CartReadModel, which wasn't restricted to a specific user role
@boostercloud boostercloud locked as resolved and limited conversation to collaborators Nov 3, 2020
@boostercloud boostercloud unlocked this conversation Nov 3, 2020
@boostercloud boostercloud locked as resolved and limited conversation to collaborators Nov 3, 2020
@boostercloud boostercloud unlocked this conversation Nov 3, 2020
@boostercloud boostercloud locked as resolved and limited conversation to collaborators Nov 3, 2020
@javiertoledo javiertoledo merged commit a696750 into master Nov 3, 2020
@javiertoledo javiertoledo deleted the fix/integration-tests branch November 3, 2020 22:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

feature-request New feature or request hacktoberfest-accepted size: L Tasks that imply many files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLI integration tests delete files from the repo Integration test for CLI don't work

4 participants