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

Stop preconditions from being executed twice #130

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

lstrzebinczyk
Copy link
Contributor

@lstrzebinczyk lstrzebinczyk commented Aug 22, 2024

When running an action with #try_perform!, preconditions are being run twice. This PR fixes this.

https://github.com/toptal/platform/pull/78833 is a platform PR run against this patch, with successfull unit tests and features:

Devx-1752-test-granite-by-lstrzebinczyk-·-Pull-Request-78833-·-toptal-platform
Devx-1752-test-granite-by-lstrzebinczyk-·-Pull-Request-78833-·-toptal-platform(1)

How to test

class TestAction < BaseAction 
  precondition do
    puts "RUNNING PRECONDITION"
  end

  subject :role_step
  
  history false

  private

  def execute_perform!(*)
  end
end
  • run TestAction.as_system.new(RoleStep.new).try_perform!
  • in your console, you should see RUNNING PRECONDITION twice
  • run the same on master, you'll see it twice.

Review

Pre-merge checklist

  • The PR relates to a single subject with a clear title and description in grammatically correct, complete sentences.
  • Verify that feature branch is up-to-date with master (if not - rebase it).
  • Double check the quality of commit messages.
  • Squash related commits together.

@lstrzebinczyk lstrzebinczyk force-pushed the devx-1752-fix-preconditions-in-granite branch from a1c92ed to b163b33 Compare August 23, 2024 10:15
@lstrzebinczyk lstrzebinczyk self-assigned this Aug 23, 2024
@lstrzebinczyk lstrzebinczyk marked this pull request as ready for review August 23, 2024 13:14
@lstrzebinczyk lstrzebinczyk requested a review from a team as a code owner August 23, 2024 13:14
@lstrzebinczyk
Copy link
Contributor Author

@toptal-anvil ping reviewers

lib/granite/action/preconditions.rb Outdated Show resolved Hide resolved
@@ -13,7 +13,7 @@
require 'rspec/matchers/fail_matchers'
require 'simplecov'
SimpleCov.start do
minimum_coverage 99.66
minimum_coverage 99.43
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to add a new spec to ensure the preconditions are only called once, I think you can call a spy/mock in

precondition { decline_with(:message) if login == '' }
and then check is only called once in after perform

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spec added 👍

@lstrzebinczyk
Copy link
Contributor Author

@toptal-anvil ping reviewers

@lstrzebinczyk lstrzebinczyk requested review from mbie and catks August 26, 2024 09:15
@lstrzebinczyk
Copy link
Contributor Author

@toptal-anvil ping reviewers

@lstrzebinczyk lstrzebinczyk force-pushed the devx-1752-fix-preconditions-in-granite branch from 60a1cb5 to 3fce264 Compare August 26, 2024 13:55
@lstrzebinczyk lstrzebinczyk marked this pull request as draft August 28, 2024 08:51
@lstrzebinczyk
Copy link
Contributor Author

I've converted to draft to hide from seal. I'll get back to this once I have a moment, or after oncall

@lstrzebinczyk lstrzebinczyk marked this pull request as ready for review August 29, 2024 09:49
@lstrzebinczyk lstrzebinczyk merged commit 95daead into master Aug 29, 2024
5 checks passed
@lstrzebinczyk lstrzebinczyk deleted the devx-1752-fix-preconditions-in-granite branch August 29, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants