Skip to content

Assign a default status for plugins that do not use utils.status.show() #1275

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

Merged
merged 1 commit into from
May 14, 2020

Conversation

ehmicky
Copy link
Contributor

@ehmicky ehmicky commented May 14, 2020

Part of #1223.

When a plugin does not use utils.status.show(), a default success status should be used.

That status should be implicitly set by the last event handler, excluding onError and onEnd.

This PR also adds many tests covering implicit statuses and how status can override each other.

@ehmicky ehmicky added the type: feature code contributing to the implementation of a feature and/or user facing functionality label May 14, 2020
@ehmicky ehmicky requested a review from erezrokah May 14, 2020 11:23
@ehmicky ehmicky self-assigned this May 14, 2020
(Netlify Build completed in 1ms)`

## utils.status.show() implicit status is not used when there are no events
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having a hard time understanding the difference between implicit status is not used when there are no events, implicit status is not used when no call was made, with only onError and implicit status is used when no call was made, implicit status is used when no call was made, with only onEnd and implicit status is used when no events have made a call.
How is the implicit status reflected in the snapshot? With previous tests I can see the summary, but the only differences I can see in these snapshots are the logs for specific commands.
What am I missing?

Copy link
Contributor Author

@ehmicky ehmicky May 14, 2020

Choose a reason for hiding this comment

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

Sorry this is confusing, and thanks for being thorough by checking the test snapshots!

Statuses bring together several features: build errors, plugin errors, build cancellation and plugin success statuses. Also we assign a default success status when the plugin does not do it explicitly. Finally, we need to ensure that:

  • this implicit success status is not overriding a build error or explicit call to utils.status.show()
  • utils.status.show() overrides any previous call to utils.status.show()
  • utils.status.show() cannot override a plugin or build error

This rather complex logic leads to the tests being a little complicated. I have written down some more detailed information below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that the specific tests you ask for are the last 3 ones in the post below.

@ehmicky
Copy link
Contributor Author

ehmicky commented May 14, 2020

Note: the tests only checks the user-visible behavior of this feature (everything else is just implementation details):

  • creating a status:
    • calling utils.status.show() in a plugin
    • error happening in a plugin/build
  • seeing a status:
    • printed in the build logs
    • shown in the UI deploy summary, which is done by making an API call (not implemented yet)

This is a detailed explanation what each test is checking. Each point starts with the test title.

can override a success status

  • fixture success_status_override
  • onBuild() calls show({ summary: 'onBuild' }) but it is overridden by later onSuccess() show({ summary: 'onSuccess' })
  • result: logs print { summary: 'onSuccess' } in the final summary

cannot override an error status with a success status

  • fixture error_status_override
  • onBuild() throws an error onBuild. onSuccess(), onError() and onError() call show() but this should be a noop because error statuses cannot be overridden by success statuses
  • result: logs print the error onBuild and no success statuses in the final summary

can override an error status with another error status

  • fixture error_status_error_override
  • onBuild() throws an error onBuild but it is overriden by onError() throwing onError later on
  • result: logs print the error onError but not the error onBuild in the final summary

implicit status is not used when an explicit call was made

  • fixture no_implicit
  • onBuild() calls show({ summary: 'summary' }). Later on, onSuccess() calls nothing, so the success status from onBuild() should be used.
  • result: logs print { summary: 'summary' } in the final summary

implicit status is not used when there are no events

  • fixture no_implicit_none
  • plugin without any events should not report any statuses
  • result: logs print no statuses in the final summary

implicit status is not used when no call was made, with only onError

  • fixture no_implicit_onerror
  • plugin with an onError() event handler that does not get triggered should report no statuses
  • result: logs print no statuses in the final summary

implicit status is used when no call was made

  • fixture implicit_one
  • plugin that do not throw any error nor call show() should report a default "implicit" success status
  • result: "implicit" success statuses are not printed in the console. They are only sent to the API. Since API calls is not implemented yet, the snapshot only shows that nothing is printed in the logs. However when API calls for statuses are implemented, I will update this test to check the API call was made with the "implicit" success status.

implicit status is used when no events have made a call

  • fixture implicit_several
  • same as previous one, except with several event handlers

implicit status is used when no call was made, with only onEnd

  • fixture implicit_onend
  • same as previous one, except with only one onEnd() event handler

@ehmicky ehmicky requested a review from erezrokah May 14, 2020 13:14
@erezrokah
Copy link
Contributor

Thanks for the detailed explanation, I get the desired behaviour and each test implementation.
What was confusing for me and now is clear is that the user visible behaviour (and the behaviour that is visible in the snapshot) is the same for implicit status is not used * tests and implicit status is used * tests. Adding a validation the API is called in the future would be great.

@ehmicky ehmicky merged commit f6b100c into master May 14, 2020
@ehmicky ehmicky deleted the feat/no-status branch May 14, 2020 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants