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

Implement assertions for Minitest #2

Merged
merged 7 commits into from
Sep 16, 2021

Conversation

chriscz
Copy link
Contributor

@chriscz chriscz commented Sep 23, 2020

What is the purpose of this pull request?

This pull request tracks an implementation of some minitest assertions & expectations.

What changes did you make? (overview)

So far I've added the ActiveEventStore::Minitest::Assertions module and the assert_published method and I plan on adding an assert_enqueued_async_subscriber method as well.

Is there anything you'd like reviewers to focus on?

Nothing for review, but some questions:

  1. How would you prefer I test these assertions?
  2. Any convention or templates from other Evil Martian projects that I should follow here?

Checklist

  • I've implemented assert_published
  • I've implemented assert_enqueued_async_subscriber
  • I've added assertions as minitest expectations
  • I've refactored it into multiple modules
  • I've added tests for this change
  • I've added a Changelog entry
  • I've updated a documentation

@palkan
Copy link
Owner

palkan commented Sep 23, 2020

Thanks for you contribution!

How would you prefer I test these assertions?

Check out, for example: https://github.com/palkan/action_policy/blob/master/test/action_policy/test_helper_test.rb

Any convention or templates from other Evil Martian projects that I should follow here?

Let's define assertions in the ActiveEventStore::TestHelper file—that's a common practice used in Rails itself and other libraries.

I've implemented assert_published

What about a less generic name? Say, assert_event_published (similarly to RSpec have_published_event).

@chriscz
Copy link
Contributor Author

chriscz commented Sep 29, 2020

Hi @palkan I've made some of the changes you suggested. I can't help but notice the similarity between the rspec and minitest matchers and I'm wondering if a refactor would make sense?

When I run the tests using rake test I noticed that my import of bundler prints out a lot of deprecation notices and the test run seems very noisy. I can't seem to go without importing bundler as I get an error if I don't. I don't have much time to investigate tonight, but here's an example of the output so long:

WARN: Unresolved specs during Gem::Specification.reset:
      minitest (~> 5.1)
      rails-html-sanitizer (>= 1.2.0, ~> 1.0, ~> 1.1)
      nokogiri (>= 1.6)
      builder (~> 3.1)
      erubi (~> 1.4)
      rake (>= 0.8.7)
      method_source (>= 0)
WARN: Clearing out unresolved specs.
Please report a bug if this causes problems.
/home/chris/.rbenv/versions/2.5.8/lib/ruby/2.5.0/json/version.rb:4: warning: already initialized constant JSON::VERSION
/home/chris/.rbenv/versions/2.5.8/lib/ruby/gems/2.5.0/gems/json-2.3.1/lib/json/version.rb:4: warning: previous definition of VERSION was here
/home/chris/.rbenv/versions/2.5.8/lib/ruby/2.5.0/json/version.rb:5: warning: already initialized constant JSON::VERSION_ARRAY
/home/chris/.rbenv/versions/2.5.8/lib/ruby/gems/2.5.0/gems/json-2.3.1/lib/json/version.rb:5: warning: previous definition of VERSION_ARRAY was here
/home/chris/.rbenv/versions/2.5.8/lib/ruby/2.5.0/json/version.rb:6: warning: already initialized constant JSON::VERSION_MAJOR
/home/chris/.rbenv/versions/2.5.8/lib/ruby/gems/2.5.0/gems/json-2.3.1/lib/json/version.rb:6: warning: previous definition of VERSION_MAJOR was here
/home/chris/.rbenv/versions/2.5.8/lib/ruby/2.5.0/json/version.rb:7: warning: already initialized constant JSON::VERSION_MINOR
/home/chris/.rbenv/versions/2.5.8/lib/ruby/gems/2.5.0/gems/json-2.3.1/lib/json/version.rb:7: warning: previous definition of VERSION_MINOR was here
/home/chris/.rbenv/versions/2.5.8/lib/ruby/2.5.0/json/version.rb:8: warning: already initialized constant JSON::VERSION_BUILD
/home/chris/.rbenv/versions/2.5.8/lib/ruby/gems/2.5.0/gems/json-2.3.1/lib/json/version.rb:8: warning: previous definition of VERSION_BUILD was here
/home/chris/.rbenv/versions/2.5.8/lib/ruby/2.5.0/json/common.rb:100: warning: already initialized constant JSON::NaN
/home/chris/.rbenv/versions/2.5.8/lib/ruby/gems/2.5.0/gems/json-2.3.1/lib/json/common.rb:107: warning: previous definition of NaN was here
/home/chris/.rbenv/versions/2.5.8/lib/ruby/2.5.0/json/common.rb:102: warning: already initialized constant JSON::Infinity
/home/chris/.rbenv/versions/2.5.8/lib/ruby/gems/2.5.0/gems/json-2.3.1/lib/json/common.rb:109: warning: previous definition of Infinity was here
/home/chris/.rbenv/versions/2.5.8/lib/ruby/2.5.0/json/common.rb:104: warning: already initialized constant JSON::MinusInfinity
/home/chris/.rbenv/versions/2.5.8/lib/ruby/gems/2.5.0/gems/json-2.3.1/lib/json/common.rb:111: warning: previous definition of MinusInfinity was here
/home/chris/.rbenv/versions/2.5.8/lib/ruby/2.5.0/json/common.rb:129: warning: already initialized constant JSON::UnparserError
/home/chris/.rbenv/versions/2.5.8/lib/ruby/gems/2.5.0/gems/json-2.3.1/lib/json/common.rb:136: warning: previous definition of UnparserError was here
      create  db/migrate/20200929192123_create_event_store_events.rb
Run options: --seed 24960

# Running:

.......

Finished in 0.110304s, 63.4607 runs/s, 81.5924 assertions/s.
7 runs, 9 assertions, 0 failures, 0 errors, 0 skips

@chriscz
Copy link
Contributor Author

chriscz commented Sep 30, 2020

I got the output cleared by running bundle clean --force so it looks like an issue with my local gems, also llooks like I forgot to run with bundle exec 🤦

@chriscz chriscz force-pushed the feature/add-minitest-assertions branch from def9cee to fcf3269 Compare December 10, 2020 08:03
@mostlyobvious
Copy link

👋

For the record — there's an interest for minitest matchers in upstream too:

Perhaps we can team up on this?

@chriscz
Copy link
Contributor Author

chriscz commented Sep 9, 2021

Hey @pawelpacana sure thing! In a recent project we've been using some helpers which might be of use in RES: https://gist.github.com/chriscz/14a8b5c9874706a9d8c07fe50041f2b1

@palkan
Copy link
Owner

palkan commented Sep 14, 2021

Hey @chriscz!
Sorry, are you waiting on some feedback from my side? Or we're ready to merge it?

(Probably, we should wait for #6 first)

@chriscz
Copy link
Contributor Author

chriscz commented Sep 15, 2021

Hey @palkan I'm rounding this off tonight will mark ready for review then. Just need to update the docs and I'll be done.

@chriscz chriscz force-pushed the feature/add-minitest-assertions branch from e745515 to 6713694 Compare September 16, 2021 05:38
@chriscz chriscz marked this pull request as ready for review September 16, 2021 05:38
@chriscz chriscz force-pushed the feature/add-minitest-assertions branch from 6713694 to 1cc2b90 Compare September 16, 2021 05:47
@chriscz chriscz force-pushed the feature/add-minitest-assertions branch 2 times, most recently from 539c286 to b0f8228 Compare September 16, 2021 06:16
Copy link
Owner

@palkan palkan left a comment

Choose a reason for hiding this comment

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

Awesome work! Thanks!

Left a couple comments.

.github/workflows/minitest.yml Outdated Show resolved Hide resolved
Rakefile Outdated Show resolved Hide resolved
gemfiles/rubocop.gemfile.lock Outdated Show resolved Hide resolved
@chriscz chriscz force-pushed the feature/add-minitest-assertions branch from b0f8228 to 9c3df6f Compare September 16, 2021 12:36
@palkan palkan merged commit 968f338 into palkan:master Sep 16, 2021
@palkan
Copy link
Owner

palkan commented Sep 16, 2021

Released in 1.0.1.

Thank you!

@chriscz chriscz deleted the feature/add-minitest-assertions branch November 28, 2022 12:35
@chriscz chriscz restored the feature/add-minitest-assertions branch November 29, 2022 13:31
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.

3 participants