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(e2e): turn repeated test (build and start) into function #312

Closed
wants to merge 1 commit into from

Conversation

Jiansen
Copy link
Contributor

@Jiansen Jiansen commented Aug 1, 2016

There are 3 build-and-start tests in the current e2e.sh.

I placed repeated code in a function.

In the first test, if there is a reason why npm start -- --smoke-test should run before npm run build, I will extract npm start -- --smoke-test out of function build_check.

@ghost ghost added the CLA Signed label Aug 1, 2016
@mxstbr
Copy link
Contributor

mxstbr commented Aug 1, 2016

TIL you can have functions in bash scripts!

@Jiansen
Copy link
Contributor Author

Jiansen commented Aug 1, 2016

TIL you can have functions in bash scripts!

@mxstbr , I leant from @ltk ( #244 ) just a few days ago :-)

@@ -28,6 +28,30 @@ function handle_exit {
exit
}

function build_check {
local test_snap_path="$1"
if [ -z "$1" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

When would it be empty? We seem to always pass it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we always passed in snap file path in the 3 tests.

I am thinking what the build_check should behave when it is called without given $1.

-- option 1: delete the if check, let the function fall and let developer knows that Jest test is mandatory
-- option 2: use the following code, skip Jest test when there is no $1

if [ -z "$1" ]
    then
      test -e $1
fi

I will update the code when received your suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is just an end-to-end test that we run for this tool, the developer won’t see it.
So relying on jest existing is fine here.

@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

Thanks again for the PR. Sorry it’s getting out of date. There are a few more important changes I wanted to get in first but I’m keeping an eye on this one. It should be good to go after #419 is merged so I’ll get back to it and ask you to update 😄 . Thank you!

@Jiansen
Copy link
Contributor Author

Jiansen commented Sep 2, 2016

This PR is to clean up e2e.sh. It makes sense to do this after other important changes.

@gaearon
Copy link
Contributor

gaearon commented Sep 30, 2016

I’m closing because this got out of date.
I’ll be happy to merge it if you rebase, so feel free to send another PR!
Thank you for your contributions.

@gaearon gaearon closed this Sep 30, 2016
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants