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

Added small script to test installation on with different dependencies #157

Merged
merged 4 commits into from
Dec 27, 2019

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Dec 27, 2019

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets Fix #156
Documentation
License MIT

This is a small script to make sure we test different sets of configuration. Related to #156

Test should fail because #156 describes a problem.

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks, good idea!

can we improve output of the build? its rather cryptic now to see what is not working. and do you plan on adding the fix in this PR too?

.travis.yml Outdated Show resolved Hide resolved
echo -e "\\e[41mKO\\e[0m $title\\n"
(exit $ok)
}
export -f install_test
Copy link
Contributor

Choose a reason for hiding this comment

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

could we add this as a bash file to tests folder, or .travis folder? no big deal, but it seems a bit much for the travis configuration file

Copy link
Member Author

Choose a reason for hiding this comment

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

I dont think so. Im not sure how you can "export" a function in bash/travis.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah i see, bash function, not bash script. i guess thats fine then. maybe bash source-ing the file would work, but i will merge as is and tag another patch release

@Nyholm
Copy link
Member Author

Nyholm commented Dec 27, 2019

Sure, I added a commit with a fix for #156

@Nyholm
Copy link
Member Author

Nyholm commented Dec 27, 2019

can we improve output of the build?

What do you suggest?

  • Each test is folded so it get easier to see what is what.
  • Each tests ends with a green "OK" or a red "KO" to signal status.
  • The second last line of the tests prints some output that is helpful.

@dbu
Copy link
Contributor

dbu commented Dec 27, 2019

i somehow expected something before the error line, but the last line is enough. as it should normally not fail, it will at least indicate when something is amiss.

@dbu dbu merged commit e95c211 into php-http:master Dec 27, 2019
@Nyholm
Copy link
Member Author

Nyholm commented Dec 27, 2019

Thank you for merging

@Nyholm Nyholm deleted the install-test branch December 27, 2019 20:01
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.

[Symfony 4.4] No PSR-17 response factory found
2 participants