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

UI-309 Update output for service:checks plan error #1178

Merged
merged 6 commits into from
Apr 9, 2019

Conversation

justinanastos
Copy link
Contributor

@justinanastos justinanastos commented Apr 8, 2019

Also fixes pluralization in the Listr status messages.

All errors thrown will cause Listr to look strange. I was unable to quickly diagnose where the error originates. It exists in prod also so I didn't try too hard to fix it.

TODO:

  • Update CHANGELOG.md* with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

This will also allow us to determine the validation task name before we
define the tasks so we don't have to change the task name after we start
running.

This required changing whitespace on many lines; review this commit with
whitespace turned off.
Copy link
Contributor

@evans evans left a comment

Choose a reason for hiding this comment

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

Glad we're fixing this up! A comment. Also is there a way to test the screenshot below?

image

packages/apollo/src/commands/service/check.ts Show resolved Hide resolved
@@ -287,10 +287,6 @@ export default class ServiceCheck extends ProjectCommand {
// Save the output because we're going to use it even if we throw. `runTasks` won't return
// anything if we throw.
Object.assign(taskOutput, ctx);

task.title = `Validated local schema against tag ${chalk.blue(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is still valuable for the tense change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call.

@justinanastos
Copy link
Contributor Author

@evans

Also is there a way to test the screenshot below?

We could refactor the functions that generate the messages out and then unit test them. I think we'd be better served spending a few hours to put together e2e tests. I think if we treat this like JavaScript that we can test all the output with relative ease!

We were calculating it when the task started running before; that was
unncessary because we have all the information we need at load time.
For some reason Listr will show a few items more than once. This is not a
regression.
@justinanastos justinanastos force-pushed the justin/service-check/current-plan-error branch from 90b7ea1 to ccded7c Compare April 9, 2019 17:56
@justinanastos justinanastos requested a review from evans April 9, 2019 17:57
@justinanastos
Copy link
Contributor Author

@evans I updated the tense change like you requested; that's the only change.

https://github.com/apollographql/apollo-tooling/pull/1178/files#diff-c61805bdfe85787383d4ae9927bd0774R291

Copy link
Contributor

@evans evans left a comment

Choose a reason for hiding this comment

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

Noice! Let's ship it

I agree with your assessment that end-2-end tests would be valuable. Do you think that it's worth tracking as a ticket in Jira?

@justinanastos
Copy link
Contributor Author

@evans

I agree with your assessment that end-2-end tests would be valuable. Do you think that it's worth tracking as a ticket in Jira?

I sure do

@justinanastos justinanastos merged commit 9d4a4ef into master Apr 9, 2019
@justinanastos justinanastos deleted the justin/service-check/current-plan-error branch April 9, 2019 18:38
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.

2 participants