Bats file detik and support

[MichaelDimmitt]

@martin-schulze-vireso , here are the remaining formulas

[MichaelDimmitt]

As mentioned in #3 , not sure if detik will work
however if it does not work can be fixed in a future pr.

we need to merge the pr to test.

[MichaelDimmitt]

@martin-schulze-vireso , any issues with this pr that I can clear up?

[martin-schulze-vireso]

Can we run the tests in a GitHub Action?

[martin-schulze-vireso]

@martin-schulze-vireso , any issues with this pr that I can clear up?

I forgot to subscribe to notifications so I just did not see it.

[MichaelDimmitt]

Great suggestions, I will try to get these changes in the next few days.

[MichaelDimmitt]

Some documentation on testing taps.
https://stackoverflow.com/a/20431998/5283424

Some good details on how to create a tap
https://brew.sh/2020/11/18/homebrew-tap-with-bottles-uploaded-to-github-releases

[MichaelDimmitt]

@martin-schulze-vireso ,

1. can you take a look I have added the requested updates.
2. would it make sense to have the github action effort as a seperate pr ?

the stack overflow I posted was about using travis ci.
and on hand I do not know how

currently trying to run the tests locally with

{
  PACKAGE=bats-assert;
  brew tap bats-core/bats-core;
  brew audit $PACKAGE;
  brew install -v $PACKAGE;
  brew test $PACKAGE;
  brew developer off;
}

will report my findings shortly.

Error: bats-core/bats-core/bats-assert: failed
An exception occurred within a child process:
  BuildError: Failed executing: bats /usr/local/Cellar/bats-assert/v2.0.0/lib/bats-assert/test

the test for bats-assert failed but then I read the documentation it looks like the tests in bats-assert are only supported when installing bats-support using npm?

bats-core/bats-assert#38

should the github action be on the release itself ?
i.e. https://github.com/bats-core/bats-assert/actions

would make a green checkbox here:
https://github.com/bats-core/bats-assert/releases

lastly the model I used as a template does not have github actions setup
https://github.com/kaos/homebrew-shell/actions

to breaking new ground on this which could be time consuming.

[martin-schulze-vireso]

I'd prefer to have a test before releasing to avoid releasing something broken. We don't need to run the module's selftests though. It would be sufficient to run a small test that shows we successfully loaded the dependency.

[MichaelDimmitt]

It does not exist until it is merged to master
not sure how to achieve what you are asking.

Maybe some help is needed.

[martin-schulze-vireso]

I understood that you need an established GHA workflow on master to add your changes to it. I commited a dummy workflow to master. Just rebase and you should be good to go.

[brokenpip3]

I can help if you like and create that simple github action, will push something later today :)

[brokenpip3]

Ok I did a first working action: https://github.com/brokenpip3/homebrew-bats-core/blob/main/.github/workflows/test-brew-formulas.yaml and this is the last run: https://github.com/brokenpip3/homebrew-bats-core/runs/4245164444?check_suite_focus=true

Please be aware:

• I split the 4 libs (support, assert, file, detik) in 3 different sub-steps: audit, install, test to let you guys catch all the errors, otherwise the gh action will fail at first one
• As you can see we have different errors on audit and test steps.
• Also we have some problems to install bats-core with the official brew formula, see this error, it's not the scope of this PR but will open a bug later

@MichaelDimmitt let me know if you want a PR against your branch or you can just copy and paste from my fork.
Unfortunately I'm not a mac-os user/owner, so I can't help you more but if you have any other questions on github actions or change request please let me know (I'm trying to have this PR approved and merged because I want to enable my team to use bats and bats-libs, so let me know if I can do more)

[brokenpip3]

Just updated my branch with last @MichaelDimmitt changes, you can see the logs here: https://github.com/brokenpip3/homebrew-bats-core/runs/4249308567?check_suite_focus=true and opened the PR against Michael's fork: MichaelDimmitt#10

[brokenpip3]

@MichaelDimmitt any news? Can I help you with something else?

[MichaelDimmitt]

Thanks @brokenpip3 for the save!

I have merged in your changes and made some updates to pass the audit. I removed the continue on error for all tasks except the test actions.

@martin-schulze-vireso can you take a look at the github action, are we ready to merge or are more changes needed? https://github.com/bats-core/homebrew-bats-core/runs/4457052834?check_suite_focus=true

Cheers, Michael Dimmitt

[MichaelDimmitt]

squashed commits

[martin-schulze-vireso]

I think it is unnecessary to run the libraries' self tests to demonstrate that the installation works.

I'd much rather have an example script for each library that uses the library.

E.g. the bats-assert example script could look something like:

setup() {
    load '#{HOMEBREW_PREFIX}/lib/bats-support/load.bash'
    load '#{HOMEBREW_PREFIX}/lib/bats-assert/load.bash'
}

@test test {
    assert true
}

and then try to run that in the tests.

[brokenpip3]

I think it is unnecessary to run the libraries' self tests to demonstrate that the installation works.

imho the real question here is: why the standard libraries tests fail on macos? we should care about it?

I'd much rather have an example script for each library that uses the library.

E.g. the bats-assert example script could look something like:

setup() {
    load '#{HOMEBREW_PREFIX}/lib/bats-support/load.bash'
    load '#{HOMEBREW_PREFIX}/lib/bats-assert/load.bash'
}

@test test {
    assert true
}

and then try to run that in the tests.

this is ok for support instead for detik at the end we should do something like https://github.com/bats-core/bats-detik/blob/6ccb9f64e33d11f9658d6a38553b383225f0ccef/tests/test.detik.try.to.find.bats#L8-L22 right?
that's why the fact that the upstream tests does not work on macos is puzzling me
for instance bats-support tests are OK, bats assert ones goes into error without any failing test, bats file and detik instead has some problems, if you compare it with a the same tests on linux they pass without any problem (you can see the test I'm running for a bats gh action I made https://github.com/brokenpip3/setup-bats-libs/blob/main/.github/workflows/test-local-action.yaml, https://github.com/brokenpip3/setup-bats-libs/runs/4227437090?check_suite_focus=true )
That could be related to the bats version, since with 1.5.0 I can failures even on gh linux-runner (not on my pc), unfortunately I can't test it deeply since I'm not on a mac owner.

[martin-schulze-vireso]

I think it is unnecessary to run the libraries' self tests to demonstrate that the installation works.

imho the real question here is: why the standard libraries tests fail on macos? we should care about it?

I am currently preparing a 1.5.1 bugfix release that should solve issues I have seen with bats-assert's selftest suite. Maybe the others have the same problem. I would say that this repo/PR should not concern itself with internal test failures of those libraries tests. The objective of this repo is to facilitate installing the dependencies and the tests should check that this is working. If there is a problem with the selftests that should be solved in the respective repos (or in this case in bats-core).

this is ok for support instead for detik at the end we should do something like https://github.com/bats-core/bats-detik/blob/6ccb9f64e33d11f9658d6a38553b383225f0ccef/tests/test.detik.try.to.find.bats#L8-L22 right?

Yes, we should have an extra example per library that requires usage of at least one exclusive feature of the respective library to prove that it is loaded.

[brokenpip3]

imho the real question here is: why the standard libraries tests fail on macos? we should care about it?

I am currently preparing a 1.5.1 bugfix release that should solve issues I have seen with bats-assert's selftest suite. Maybe the others have the same problem.

This is great, thanks!
for my own curiosity do we have a bug/PR so I can read the motivation behind the issue?

I would say that this repo/PR should not concern itself with internal test failures of those libraries tests. The objective of this repo is to facilitate installing the dependencies and the tests should check that this is working. If there is a problem with the selftests that should be solved in the respective repos (or in this case in bats-core).

Ok this make totally sense, I can create try to do the required changes testing it using GH actions.
Also perhaps are you interested on some PRs for each library to always run the tests on each commit/PR? I can take care of these PRs

this is ok for support instead for detik at the end we should do something like https://github.com/bats-core/bats-detik/blob/6ccb9f64e33d11f9658d6a38553b383225f0ccef/tests/test.detik.try.to.find.bats#L8-L22 right?

Yes, we should have an extra example per library that requires usage of at least one exclusive feature of the respective library to prove that it is loaded.

Ok will try to implement it like that upstream example

[martin-schulze-vireso]

I am currently preparing a 1.5.1 bugfix release that should solve issues I have seen with bats-assert's selftest suite. Maybe the others have the same problem.

This is great, thanks! for my own curiosity do we have a bug/PR so I can read the motivation behind the issue?

See bats-core/bats-core#522 but that only deals with issues under Linux. The 1.5.1 release will get a cherry pick of this.

Also perhaps are you interested on some PRs for each library to always run the tests on each commit/PR? I can take care of these PRs

Any help is welcome. I already started with bats-core/bats-assert#41 which showed the problem above. It would be good to have some automated tests on new master versions of bats-core as well. I am not sure if we should invert that and let bats-core do those integration tests in a central place.

[brokenpip3]

Any help is welcome. I already started with bats-core/bats-assert#41 which showed the problem above. It would be good to have some automated tests on new master versions of bats-core as well. I am not sure if we should invert that and let bats-core do those integration tests in a central place.

Ok let me move this conversation on that PR

[MichaelDimmitt]

I resolved one of the threads,
Little confused on what is remaining.

Can we post an outline of what changes are remaining for getting this pr across the finish line?
Thank you

[martin-schulze-vireso]

I'd say:

1. Resolve all open conversations from the last review
2. Use dedicated example test files for each library for the test
3. Get green tests without ignoring failures

[brokenpip3]

Ok I added all the changes requested, @MichaelDimmitt could you please approve this PR: MichaelDimmitt#11 so @martin-schulze-vireso can review my last changes?

summary:

• added simple tests for bats-assert, bats-file and bats-detik
• removed all the "continue-on-error: true"
• changed the bats-detik path: bats detik is a bit different since it does not use the classic "load.bash" instead the library are separated and self-contains, imho we should avoid to add an another "lib" directory there and instead mv the 2 library files directly under the bats-detik directory. Even in arch-linux I did the same consideration.

Now we should be close to merge this PR 🎉

[MichaelDimmitt]

Thanks @brokenpip3 , merged your change that had the tests that he requested.
and the pipeline working without continue-on-error.

Looks like we should be good for merge. @martin-schulze-vireso

[martin-schulze-vireso]

Some nits but I won't hold this up any longer:

1. is it common to put the test files into the formula instead of having them "on disk" as separate files?
2. the tests seem to work but I don't see any bats output. Can we get verbose output to see if everything works as expected?
3. I'd give each library its own job (maybe using a matrix?) instead of steps in the same job.

[brokenpip3]

Some nits but I won't hold this up any longer:

1. is it common to put the test files into the formula instead of having them "on disk" as separate files?

https://docs.brew.sh/Formula-Cookbook#add-a-test-to-the-formula
example they suggest: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/tinyxml2.rb

2. the tests seem to work but I don't see any bats output. Can we get verbose output to see if everything works as expected?

3. I'd give each library its own job (maybe using a matrix?) instead of steps in the same job.

Ok np, Let me work on these ^ on another PR today
The only caveat is that for assert and file we still need to install support, but I will do it with a needs

[martin-schulze-vireso]

3. I'd give each library its own job (maybe using a matrix?) instead of steps in the same job.

Ok np, Let me work on these ^ on another PR today The only caveat is that for assert and file we still need to install support, but I will do it with a needs

Isn't this one of the features of brew to install the transitive dependency as well?

[brokenpip3]

3. I'd give each library its own job (maybe using a matrix?) instead of steps in the same job.

Ok np, Let me work on these ^ on another PR today The only caveat is that for assert and file we still need to install support, but I will do it with a needs

Isn't this one of the features of brew to install the transitive dependency as well?

yes but in case of new release we should install the local formula version, right?
Let's image a new PR: If bats-support will never have new version we can always install it from this public repo (with brew tap) using the brew install assert|file directly.
Instead in case of new versions we should use the local (repo) formula that will contains the new bats-support version and test it with assert|file before pushing it into main.
So we should decide if we want to rely on support from main (this repo by using brew install assert|file directly) or the local version (this repo but from different branch that could contains a new version by using brew install --build-from-source <local-formula-file)
Make sense?