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.

Sign up for GitHub

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

Jump to bottom

refactor(cli/js/testing): Add a tests namespace #4387

Closed
wants to merge 7 commits into from
Closed

refactor(cli/js/testing): Add a tests namespace #4387

wants to merge 7 commits into from

Conversation

nayeemrmn
Copy link
Collaborator

@nayeemrmn nayeemrmn commented Mar 16, 2020 •
edited
Loading

Ref #4351 (comment).

  • TestStatus -> tests.Status
  • TestResult -> tests.Result
  • TestEventStart -> tests.StartMessage
    • Removed field kind.
    • Field tests: number -> tests: TestDefinition[].
  • TestEventTestStart -> tests.TestStartMessage
    • Removed field kind.
    • Field name: string -> test: TestDefinition.
  • TestEventTestEnd -> tests.TestEndMessage
    • Removed field kind.
  • TestEventEnd -> tests.EndMessage
    • Removed field kind.
  • TestStats -> tests.Stats
  • TestReporter -> tests.Reporter
  • ConsoleTestReporter -> tests.ConsoleReporter

I removed the kind field from the message interfaces because it is redundant in the Reporter API... then I saw it was being used for the JSON message passing in unit_test_runner.ts. It's still correct to remove it from the message interface so I made it external.

cc @bartlomieju

@nayeemrmn nayeemrmn changed the title refactor(cli/js/testing): Add a tests namespace WIP: refactor(cli/js/testing): Add a tests namespace Mar 16, 2020
@nayeemrmn nayeemrmn changed the title WIP: refactor(cli/js/testing): Add a tests namespace refactor(cli/js/testing): Add a tests namespace Mar 16, 2020
@nayeemrmn
Copy link
Collaborator Author

@bartlomieju Looks like there has been std test failures since #4381. They're not causing CI to fail. I tried deno test module-with-failing-test.ts just now on v0.36.0 and got a 0 exit code...

@bartlomieju
Copy link
Member

@bartlomieju Looks like there has been std test failures since #4381. They're not causing CI to fail. I tried deno test module-with-failing-test.ts just now on v0.36.0 and got a 0 exit code...

Thanks for the heads up; I've opened #4392. We really need to get rid of std/testing/runner.ts and run all tests using deno test

bartlomieju
bartlomieju reviewed Mar 16, 2020
View reviewed changes
cli/js/lib.deno.ns.d.ts Show resolved Hide resolved
cli/js/lib.deno.ns.d.ts Show resolved Hide resolved
cli/js/lib.deno.ns.d.ts Show resolved Hide resolved
cli/js/tests/test_util.ts Show resolved Hide resolved
bartlomieju
bartlomieju reviewed Mar 18, 2020
View reviewed changes
cli/js/testing.ts Outdated Show resolved Hide resolved
@ry
Copy link
Member

ry commented Mar 18, 2020

waiting on #4373

@nayeemrmn nayeemrmn force-pushed the tests-namespace branch 2 times, most recently from ce0ff22 to 4ad9df0 Compare March 19, 2020 01:15
@bartlomieju
Copy link
Member

@nayeemrmn I'll rebase and merge this PR after landing #4392 and #4397

bartlomieju
bartlomieju reviewed Mar 19, 2020
View reviewed changes
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, waiting on @ry's approval to land

@nayeemrmn
Copy link
Collaborator Author

@ry Please review.

ry
ry reviewed Mar 21, 2020
View reviewed changes
cli/js/lib.deno.ns.d.ts
testStart(message: TestStartMessage): Promise<void>;
testEnd(message: TestEndMessage): Promise<void>;
end(message: EndMessage): Promise<void>;
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't like that the test interface has become so complex that we now need to introduce a namespace to keep all the associated classes and interfaces.

It used to be just a single function.

Maybe we should work on simplifying it rather than resorting to namespaces.

Also should it be "tests" or "testing"? In std we have "std/testing".

Copy link
Member

Choose a reason for hiding this comment

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

The test interface could be simplified greatly. Reporter could be a single function instead of a class. All of the message types could be reduced into one.

Copy link
Member

Choose a reason for hiding this comment

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

ConsoleReporter doesn't need to be exposed to the public at all.

@ry
Copy link
Member

ry commented Mar 21, 2020 •
edited
Loading

@nayeemrmn the patch is fine, I appreciate the work, but this namespace allows the tests namespace to be more complex than it should. I want it to be ugly so are forced to simplify it back into a single function.

Closing without merge.

@ry ry closed this Mar 21, 2020
@ry
Copy link
Member

ry commented Mar 21, 2020

I have added #4450 explaining what I'd like to do instead of this patch.

@nayeemrmn
Copy link
Collaborator Author

@ry There is a bit of a cleanup here that involves getting rid of an interface, if it's not too much of a baby step could this land if I removed the namespace?

@ry
Copy link
Member

ry commented Mar 21, 2020

@nayeemrmn yes, happy to land cleanups. Please start a new PR and ref #4450

@nayeemrmn nayeemrmn deleted the tests-namespace branch March 21, 2020 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartlomieju bartlomieju bartlomieju left review comments

@ry ry ry left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nayeemrmn @bartlomieju @ry