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

ProjectM 3.1.12 (new formula) #71139

Closed
wants to merge 1 commit into from
Closed

Conversation

revmischa
Copy link
Contributor

@revmischa revmischa commented Feb 14, 2021

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@BrewTestBot BrewTestBot added deprecated license Formula uses a deprecated SPDX license which should be updated new formula PR adds a new formula to Homebrew/homebrew-core labels Feb 14, 2021
@revmischa revmischa marked this pull request as draft February 14, 2021 16:31
@BrewTestBot BrewTestBot removed the deprecated license Formula uses a deprecated SPDX license which should be updated label Feb 14, 2021
Formula/projectm.rb Outdated Show resolved Hide resolved
@bayandin
Copy link
Member

Getting an error on brew test projectm, not sure what the meaning is

Formula requires real test block instead of current stub

@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Feb 14, 2021
@revmischa
Copy link
Contributor Author

Not really sure what I'm doing wrong in my tests

@SMillerDev
Copy link
Member

Not really sure what I'm doing wrong in my tests

The test should be a command a user would issue. And check if the output is as expected. You're currently just checking if files exist (and they don't all exist).

@revmischa
Copy link
Contributor Author

I don't understand why the exists check fails though

@SMillerDev
Copy link
Member

I don't understand why the exists check fails though

It doesn't really matter why, it's not a sufficient test either way so it'll have to be replaced.
We need a test that exercises the some of the functionality of the app.

In most cases, a good test would involve running a simple test case: run #{bin}/foo input.txt.

  • Then you can check that the output is as expected (with assert_equal or assert_match on the output of shell_output)
  • You can also check that an output file was created, if that is expected: assert_predicate testpath/"output.txt", :exist?

Some advice for specific cases:

  • If the formula is a library, compile and run some simple code that links against it. It could be taken from upstream's documentation / source examples.
  • If the formula is for a GUI program, try to find some function that runs as command-line only, like a format conversion, reading or displaying a config file, etc.
  • If the software cannot function without credentials, a test could be to try to connect with invalid credentials (or without credentials) and confirm that it fails as expected.
  • Same if the software requires a virtual machine, docker instance, etc. to be running.

@BrewTestBot BrewTestBot removed the automerge-skip `brew pr-automerge` will skip this pull request label Feb 20, 2021
Formula/projectm.rb Outdated Show resolved Hide resolved
@revmischa
Copy link
Contributor Author

Is this new test satisfactory?

@SMillerDev
Copy link
Member

Can you squash all your commits into one with the same title as the PR?

@SMillerDev SMillerDev changed the title ProjectM ProjectM 3.1.12 (new formula) Feb 26, 2021
@revmischa
Copy link
Contributor Author

Squashed

@revmischa revmischa marked this pull request as ready for review February 26, 2021 19:56
SMillerDev
SMillerDev previously approved these changes Feb 26, 2021
Copy link
Member

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

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

Thanks @revmischa ! Without contributions like yours it'd be impossible to keep homebrew going with the high standards that users have come to expect from the project. You can feel good knowing that you've made the world a tiny bit better for homebrew users around the world! 👍 🎉

@BrewTestBot
Copy link
Member

:shipit: @SMillerDev has triggered a merge.

@BrewTestBot
Copy link
Member

⚠️ @SMillerDev bottle publish failed.

@BrewTestBot BrewTestBot dismissed SMillerDev’s stale review February 26, 2021 20:40

bottle publish failed

@BrewTestBot
Copy link
Member

:shipit: @carlocab has triggered a merge.

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Mar 30, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Mar 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants