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.

Sign up for GitHub

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

Jump to bottom

feat: adds the missing assert_file_not_contains function #61

Merged
merged 7 commits into from
Jul 13, 2023
Merged

feat: adds the missing assert_file_not_contains function #61

merged 7 commits into from
Jul 13, 2023

Conversation

artis3n
Copy link

@artis3n artis3n commented Jul 3, 2023

I needed to use this method and I noticed the open issue that it does not yet exist. Added the assert_file_not_contains method and tests that are basically just the exact opposities of the assert_file_contains tests.

martin-schulze-vireso
martin-schulze-vireso reviewed Jul 4, 2023
View reviewed changes
Copy link
Member

@martin-schulze-vireso martin-schulze-vireso left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for this PR. Please add a test for when the file does not exist and document the desired behavior.

@artis3n
Copy link
Author

artis3n commented Jul 4, 2023 •
edited
Loading

@martin-schulze-vireso That isn't a test that exists for assert_file_contains, and seems to reproduce the work that assert_file_exists does, no?

I have a local @test 'assert_file_not_contains() <file>: returns 1 and displays path if <file> does not exist' test, but given the way assert_file_contains is written (which I mirrored in assert_file_not_contains), it returns exit code 0 alongside a grep error about the file not existing.

image

image

Do you want me to rewrite both assert_file_contains and assert_file_not_contains entirely to handle missing file cases? I'd prefer an expectation that an end user runs the assert_file_exists check in their test.

Edit: And that's actually my intention as an end user, in my @test I have assert_file_exists, assert_file_not_empty and then I want to add assert_file_not_contains and together that is my test case.

@martin-schulze-vireso
Copy link
Member

martin-schulze-vireso commented Jul 5, 2023 •
edited
Loading

My expectation would be that assert_file_(not_)contains tests for the file's existance as well, so it would be better to at least explicitly document this, better yet to check it: assert_file_contains implicitly fails if there is no file but assert_file_not_contains succeeds when the file does not exist. This means tests with typos in the filepath might succeed by accident.

@artis3n
Copy link
Author

artis3n commented Jul 5, 2023

Got it, that makes sense. Would you prefer I invoke assert_file_exists from the assert_file_not_contains function, duplicate the existence check code, or do something else?

@martin-schulze-vireso
Copy link
Member

You can call it if the resulting backtrace still points to the call to assert_file_not_contains.

@artis3n
Copy link
Author

artis3n commented Jul 6, 2023

What do you think of these changes?

@artis3n
Copy link
Author

artis3n commented Jul 9, 2023

On the failed test - macos-10.15 isn't a supported runner anymore. Would you like me to bump that to macos-12?

@artis3n
Copy link
Author

artis3n commented Jul 12, 2023

Done!

@martin-schulze-vireso
Copy link
Member

Please sign your commits, this is the last missing step.

@artis3n
Copy link
Author

artis3n commented Jul 12, 2023

Done!

@martin-schulze-vireso martin-schulze-vireso merged commit cb914cd into bats-core:master Jul 13, 2023
@martin-schulze-vireso
Copy link
Member

Thanks for your contribution(and patience).

@RedLeader962 RedLeader962 mentioned this pull request Jan 22, 2024
Merged
9 tasks
RedLeader962 added a commit to norlab-ulaval/norlab-shell-script-tools that referenced this pull request Jan 22, 2024
@RedLeader962
fix: add run in TeamCity check and add bats-file missing fct (#73)
f9726fe 
# Description
### Summary:

Add logic to check if docker run is done on a teamcity server. Also add
the missing bats-file function which was merged in PR #61 at
bats-core/bats-file#61

---

# Checklist:

### Code related
- [ ] I have made corresponding changes to the documentation (i.e.:
function/class, script header, README.md)
- [x] I have commented hard-to-understand code 
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] All tests pass locally with my changes (Check `tests/README.md`
for local testing procedure)
- [x] My commit messages follow the [conventional
commits](https://www.conventionalcommits.org) specification. See
`commit_msg_reference.md` in the repository root for details

### PR creation related 
- [x] My pull request `base ref` branch is set to the `dev` branch (the
_build-system_ won't be triggered otherwise)
- [x] My pull request branch is up-to-date with the `dev` branch (the
_build-system_ will reject it otherwise)

### PR description related 
- [x] I have included a quick summary of the changes
- [x] I have indicated the related issue's id with `# <issue-id>` if
changes are of type `fix`

 ## Note for repository admins
 ### Release PR related
- Only repository admins have the privilege to `push/merge` on the
default branch (ie: `main`) and the `release` branch.
- Keep PR in `draft` mode until all the release reviewers are ready to
push the release.
- Once a PR from `release` -> `main` branch is created (not in draft
mode), it triggers the _build-system_ test
- On merge to the `main` branch, it triggers the _semantic-release
automation_
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martin-schulze-vireso martin-schulze-vireso martin-schulze-vireso approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@artis3n @martin-schulze-vireso