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

refactor: standardize indentation for e2e test scripts #3390

Merged
merged 2 commits into from
Sep 26, 2023

Conversation

ericswanson-dfinity
Copy link
Member

Description

We have some e2e test scripts that use 4-space indentation, and others that have 2-space indentation. Within some e2e test script files, we have some tests that use 4-space indentation and others that use 2-space indentation.

This PR changes all e2e test scripts to use 2-space indentation, for consistency and not having to wonder what to use when adding a new test.

How Has This Been Tested?

CI will cover it

@krpeacock
Copy link
Contributor

I'm down with the consistency, but is there any way we could enforce this rule going forward with linting?

@lwshang
Copy link
Contributor

lwshang commented Sep 26, 2023

How do you format these bats scripts?
I just tried shfmt which doesn't work.

@ericswanson-dfinity
Copy link
Member Author

How do you format these bats scripts? I just tried shfmt which doesn't work.

I tried shfmt too, which didn't work, reporting } at the end of test functions as an error.

So I replaced ^ (start of line + four spaces) with two spaces, twice, and then made some manual fixes.

@ericswanson-dfinity ericswanson-dfinity marked this pull request as ready for review September 26, 2023 17:30
@ericswanson-dfinity
Copy link
Member Author

I'm down with the consistency, but is there any way we could enforce this rule going forward with linting?

We could with super-linter if shfmt worked here, but it doesn't. (And see draft PR #3293 to add super-linter)

$ shfmt -i 2 e2e/tests-dfx/assetscanister.bash 
e2e/tests-dfx/assetscanister.bash:57:1: "}" can only be used to close a block

It would be nice, but it's beyond the scope of this PR to add CI enforcement.

@lwshang
Copy link
Contributor

lwshang commented Sep 26, 2023

Found a possible solution:
https://github.com/bats-core/bats-core/blob/f7defb94362f2053a3e73d13086a167448ea9133/docs/source/writing-tests.md#comment-syntax

bats has valid bash alternative syntax. Should we consider using it?

@ericswanson-dfinity
Copy link
Member Author

Found a possible solution: https://github.com/bats-core/bats-core/blob/f7defb94362f2053a3e73d13086a167448ea9133/docs/source/writing-tests.md#comment-syntax

bats has valid bash alternative syntax. Should we consider using it?

Sure, but not as part of this PR.

@mergify mergify bot merged commit 0249634 into master Sep 26, 2023
172 checks passed
@mergify mergify bot deleted the fmt-shell-scripts branch September 26, 2023 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants