-
Notifications
You must be signed in to change notification settings - Fork 517
Conversation
I'm in favor of including a standard library in Bats itself. I'll take a closer look at your patch tomorrow, but I think this makes a lot of sense. Thank you for the very thoughtful, well-researched and reasoned pull request. |
Some initial thoughts:
|
I'd suggest restructuring things like this:
|
|
||
```bash | ||
@test 'flunk() with pipe' { | ||
echo 'this test always fails' | flunk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the use case for piping a message to flunk
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Piping simplifies the design and implementation of how output is generated by library functions.
When a test fails, output is generated in stages. Each stage has a well-defined job and pipes its output into the next stage.
- Generate details:
{ ... }
groups together the output of multiple commands that produce the formatted details relevant to the failed test, which is then all piped into the next stage at once. - Add header and footer:
_decorate
adds the header and footer around the input received from the previous stage. - Display and fail:
flunk
redirects input toSTDERR
and returns 1, effectively causing the test to fail and the output to be displayed.
This is cleaner than the alternative of separately printing a header, body and footer, all while having all function individually redirect output to STDERR
.
See assert_success
or assert_line
for example.
@sstephenson thank you for the detailed feedback!
I agree with both. Simplifies usage quite a bit.
This cracked me up! Good one! 😄
Agreed. I'm also thinking of rewriting the documentation to be more concise (e.g. man page style), we'll see how it works out.
I completely overlooked that. Moved to Most of these changes are already done. Once I finish the man page I'll update the pull request. Hopefully by Wednesday. P.S.: Sorry for the delay! Life happened. I'm back on working on this pull request. Replies should be quicker now. :) _Edit:_ Add |
61c7689
to
010b976
Compare
Pull Request v2OverviewThanks to the great feedback of @sstephenson and @aw-altiscale, I finished implementing the first round of changes and amended the pull request. We have three different documentations now.
I'm not quite happy with the current form of the man page. It's dry and hard to read. It's not much more friendly than the developer documentation. I will rewrite it as we work out how some of the Changes
Questions
TODO
_Edit:_ Fix function name in TODO: partial and regex matching is for |
Pull Request v3OverviewI finished reorganising the output matching functionality. It took longer than I expected, sorry about that. Now, the only functions that match output are The generic To make reviewing easier, I made new commits instead of amending the previous ones. I will squash them before merging. Changes
TODO
Questions
|
Now that we are mixing up the library and moving away from the model libraries (nodenv, rbenv, etc), I'm thinking that the standard input feature in The specific line matching ( We could move from the current...
... to the simpler:
The current implementation relies on the number of arguments to figure out where does the expected output/line come from. This conflicts with options having optional arguments. @sstephenson what do you think? Can we remove STDIN from these functions? |
great PR, looking forward to use the stdlib :) May I suggest you rename As a non-native English speaker, |
Good point @thomasleveil! The name
I'm in favour of changing As a non-native myself, I had to look up this word. :) |
By the way, writing style in documentation and comments, and some other conventions have gradually changed since the first commit. I will clean up everything once we have the features worked out. |
Pull Request v4OverviewThere was no feedback for a week, so I went ahead and pushed the modifications I mentioned above. I can revert them later if that becomes necessary. Changes
TODO
Questions
|
Pull Request v5I cleaned up all tests, documentation and code. Style should be consistent now. @thomasleveil proposed some changes, but due to their scope they should be in their own separate PRs. I think the following two questions left to answer before the PR can be merged.
@sstephenson what do you think? Changes
|
from pull request 'Standard Library of Test Helpers', which will hopefully make it into upstream Bats soon: sstephenson/bats#110
Great PR, looking forward to having this library included in Bats! |
Thanks for working on this impressive pull request. Ultimately it's @sstephenson's call whether to merge this in, and he might not be able to get back to you for 2 more weeks since he's on vacation. However, if it was up to me I wouldn't merge this in for size alone. Before this, bats was 760 LoC in total. This PR alone adds 3,400 new lines. I understand that a big chunk of that is comprehensive documentation, which is really great, but still. I much more like the prospect of a separate project as an assertion library, e.g. rbenv/ruby-build#817 |
@jasonkarns At this point it wouldn't make much sense trying to merge this library into Also, the name I hope to find a fitting name that properly captures the scope. Of course, the repository I'll make will be moved into a Bats organisation when created. P.S. Sorry for the long hiatus. I'm still determined to make this library a reality. _Edit:_ Add the note on Bats organisation. -- 2016-01-29 02:41 UTC |
@schrepfler Bats will not be dependent on anything. The library is opt-in. Only when you decide to use it, your test suite becomes dependent on one of the many planned installation methods (e.g. git-submodule, vendoring, package managers). And, as @RomanSaveljev said, since @sstephenson has not been active in bats development recently, we cannot move forward with a battery included approach anyway. Perhaps after the library has matured and become widely popular (i.e. the advantages outweigh the disadvantages), we can revisit the question of inclusion. |
I'm excited to begin using the new lib and ideally replacing bats-assert. How soon do you think it will be standalone in its own repo? As for the scope, there is also bats-mock that extracts the mocking utilities from ruby-build. I'm not excited about the idea of a helper lib that is so large to include assertions, helpers and mocks. But on the other hand, given bash's lack of package management, having it all brought in at once might be appealing. Regardless, there's a whole host of discussion related to mocking that would need to occur anyway (mocks vs stubs vs spies, etc) I'd like to see this lib get its own repo so we can start using it and providing feedback. |
@jasonkarns I understand what you mean. I don't want a monolithic library either, but as you said the lack of package management makes things difficult and there's a trade-off between modularity and ease of use. Here are three possible alternatives from the slightly monolithic to the modular. Starting with the monolithic side. Alternative 1: Common helpers as coreSince Bash package management is hard, there is a core library ( This is what I proposed. UsageTo use the assertions (also installs potentially unnecessary helpers):
To use another library (also installs potentially unnecessary helpers):
Alternative 2: Assertions as coreSame as alternative 1, except that the core library ( This what @jasonkarns suggested. (Am I correct?) UsageTo use the assertions:
To use another library (also installs potentially unnecessary helpers):
Alternative 3: Fully modular architectureSame as alternative 2, except that assertions ( UsageTo use the assertions:
To use another library:
ConclusionThere's a trade-off between ease of use and modularity. We have an internal library that needs to be shared between libraries, which complicates the situation. To be honest I would go with no 3, but I thought people wouldn't like the added complexity. _Edit:_ Grammar, wording and formatting. -- 2016-01-29 20:44 UTC |
TBH, I was thinking more along the lines of option 3, but not thinking about any core/internal helpers other than standard assertions. I think 3 may be the way to go for organization, but it increases the complexity of managing all the projects. (Especially considering there won't be any simple way to handle versioning and compatibility between the libs.) Personally, my projects will need to use npm for these libs. They won't need published to npm itself; rather, I would expect they simply include a minimal package.json and I would reference them directly from github. (I'm hoping that we can actually use the same package.json to support both npm and bpkg.) But for anyone who doesn't use npm, resolving version compatibility between these various libs would be a manual effort. This theoretically makes option 1 more appealing, but it doesn't actually avoid the issue, it just reduces the number of libs to deal with. OTOH, since I don't expect a lot of churn, there probably won't be many version compatibility issues in the first place. So I'm still voting for 3. |
@jasonkarns Same here. While the handling of versioning and compatibility will not be a dream, I don't expect it to be a too heavy burden later on. Like you said, consolidating libraries would not help all that much. I have a busy week ahead, but the standalone version should be ready by Friday (hopefully earlier). Most of the changes will be in documentation. Removing man pages, rewriting the section about loading libraries, and adding installation notes. I'll add It won't be super polished at first, but I want to release early and often. I will comment here when the repos are up. |
There are more changes than I expected, but the standalone version is coming along nicely. There's however one question that didn't get enough attention before. What licence should we use? I can't imagine anyone wanting to invest in using software that may later turn out to be incompatible with his/her project. Especially that many would want to use these libraries at work to test commercial software. I think a suitable licence should be selected before releasing these libraries. Things that would help:
Help is much appreciated. If nobody has the time, I'll go the Stack Overflow and FSF routes after finishing the standalone versions. |
MIT ? |
Bats itself is under MIT, so consistency would be nice. Though I believe @mislav generally prefers CC0 |
I was thinking about the licence compatibility issue.
Is this correct? If yes, the licence we use could forbid certain projects of using the test libraries. The more permissive the licence, the larger the selection of compatible licences. But my reasoning could be incorrect. |
To my knowledge, CC0 is literally the most permissive license possible. On Wed, Feb 3, 2016 at 1:00 PM, Zoltán Tömböl notifications@github.com
|
Consistency would be nice. Users of Bats would have to be familiar with only one licence instead of two. But we don't know whether the MIT licence would forbid testing software that is covered by a non-compatible licence, e.g. CC0. To be on the safe side, I vote for CC0. The standalone version is not ready yet unfortunately. Hopefully I can finish it during the weekend. Until then, let's see if anybody else wants to chime in on the licence question. |
ReleaseAfter more than 9 month of development and nearly 7 month of discussion the test helper libraries have finally been released! The initially proposed Standard Library of Test Helpers have been broken up into two external libraries and an additional repository storing shared documentation.
Features:
Moving forward all discussion should happen on the issue tracker of the respective repository. @thomasleveil You opened some very interesting pull requests over at my fork of Bats. If you are still interested, would you please reopen them on the new repositories? ThanksThanks to everyone who contributed to this discussion. Special thanks to @sstephenson @mislav @thomasleveil @jasonkarns for their invaluable contribution. Without you guys, we wouldn't have the quality libraries we have now. |
Awesome!! Looking forward to test them with basher when I'm back from the
|
Notifications@X1011 @nskforward @ivans @gwen21 @dmp1ce @jwilder @marcondesdddi @mudler I noticed that a while ago you started using the Bats test helper library we've been developing in this pull request. Good news! The library's been finally released! Check out the announcement above to learn more! |
@ztombol I will take a look at the pull requests later this week |
See sstephenson#110 for an in-depth explanation on the rationale for these changes.
…dness-for-4.2 Fix multiple error trap firing weirdness (Bash 4.2)
Bats 1.1.0 - 2018-07-08 This is the first release with new features relative to the original Bats 0.4.0. Added: * The `-r, --recursive` flag to scan directory arguments recursively for `*.bats` files (sstephenson#109) * The `contrib/rpm/bats.spec` file to build RPMs (sstephenson#111) Changed: * Travis exercises latest versions of Bash from 3.2 through 4.4 (sstephenson#116, sstephenson#117) * Error output highlights invalid command line options (sstephenson#45, sstephenson#46, sstephenson#118) * Replaced `echo` with `printf` (sstephenson#120) Fixed: * Fixed `BATS_ERROR_STATUS` getting lost when `bats_error_trap` fired multiple times under Bash 4.2.x (sstephenson#110) * Updated `bin/bats` symlink resolution, handling the case on CentOS where `/bin` is a symlink to `/usr/bin` (sstephenson#113, sstephenson#115) * tag 'v1.1.0': (198 commits) Bats 1.1.0 bats: Replace echo with printf Extract `abort()` function travis: Remove `bats -c` wrapper travis: Enable build with default Linux image Bash Add Bash version test to Travis job. Revert "Re-add Bash version check to Docker image build" Re-add Bash version check to Docker image build Move timing test to Docker run for Linux jobs Remove version check from Docker image build Bash version via build matrix instead of script loop Fix merge error. Add return code storage for Bash version loop Add Bash version output during 'docker build' Clean up Docker image tags Add default value for Bash version Cover more Bash versions with Docker BATS_ROOT: Elide options to reset shell options BATS_ROOT: Restore comment noting issue sstephenson#113 BATS_ROOT: Use `set -P`, remove `PWD` resolution ...
Overview
This pull request introduces a Standard Library of Test Helpers that provides better feedback on failed tests to help debugging. It also argues that including this library in Bats greatly simplifies its usage.
Problem
When a test fails Bats displays a lot of useful information, e.g. the failed expression and its line number. However, in many cases additional information would be helpful. Providing relevant information to speed up debugging is an actively pursued feature. It has been requested and proposed multiple times without success. Implementations generally fall into one of the following two categories.
Modifying Bats to automatically print a static set of information, e.g.
$status
and$output
, when a test fails.Advantages:
Disadvantages:
Due to its disadvantages this approach has been rejected multiple times in the past (see report $output content when test fails #8, bats_print_failed_command: Include $output if available. #101, Dump ${output} from tests by default? #105).
Using test helpers to print only information relevant to the failure.
Advantages:
Disadvantages:
This is the recommended approach (see Assertions #5).
Solution 2 is clearly the superior, and in fact the recommended approach. Two years have passed since that recommendation but a general purpose library has never emerged.
_Note:_ To be fair, @juanibiapina shared his library of assertions in #5. However, as of 2015-07-19 the link is dead and library cannot be found on his Github account.
Standard Library of Test Helpers
At the moment projects have to develop their own library by adapting another project's test helpers if the licences permit it or writing one from scratch. Six projects on the list of projects using Bats use a test helper library. All of which are a variation of the same library.
_Note:_ I know that the list mentioned above is not up to date, but users of Bats may take the projects on that list as references when building their own test suites.
The current situation has the following problems:
A properly licensed Standard Library of Test Helpers could mitigate these problems and provide a better experience for Bats users. Despite the advantages of a general purpose library, one has never emerged. This pull request introduces the Standard Library containing the most commonly sought functions.
Including the Standard Library in Bats
Although, distributing a library of test helpers with Bats has been rejected in the early days, I believe its time to re-evaluate the question in light of the current situation.
The dependency chain of the test suit for a typical project using the included Standard library would look like the following.
Including the Standard Library in Bats would greatly simplify its usage and would put less burden on individual projects.
Additionally, having an official, universally available, easy to use Standard Library would encourage contribution.
Licensing
Regardless whether a general purpose library, like the one proposed in this pull request, is included in Bats, licensing remains an important question.
Test files are just input data to Bats. The licence of Bats does not restrict the licence of the input files or the licence of the software being tested.
However, when using test helper libraries, the licence of the libraries and the licence of the test files using them must be compatible.
Furthermore, if a test file sources a file of the project (for example to do unit tests on a library) the licences of the sourced file, the test helper library, and the test file must all be compatible.
This means that the licence of a general purpose test helper library can limit what projects may use it. A permissive licence or even the Public Domain may be necessary to allow many projects to use the library.
Summary
A Standard Library of Test Helpers helps projects to write better test suits faster. However, no such library has emerged during the past two years. This pull request introduces a library containing the most common test helpers.
Including this library in Bats, rather than hosting it in a separate repository, improves its usability, simplifies maintenance and encourages contribution.
The licence of the Standard Library has a great impact on its usability and should be carefully considered.