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

test(dataService): add basic tests for withDataService #89

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

michael-small
Copy link
Contributor

@michael-small michael-small commented Aug 28, 2024

Prerequisite to feat: add support for observables to withDataService

I said that I would make tests for withDataService featuring support for observables. However, it would be good to have existing tests for the feature as it is currently with promises before the withDataService is further extended.

Once I have a test for each method of the interface DataService with promises stubbed out then I'll bump this from a draft to a full PR. Tests are not my strong suit, so please roast these.

edit:
Progress/TODO 9/29/2024

  • Happy path state changes are done
  • Still need to
    • Test loading state
    • Test error state

@rainerhahnekamp
Copy link
Collaborator

Good idea @michael-small, I'll wait for the removal of the draft status.

@michael-small michael-small marked this pull request as ready for review September 1, 2024 19:15
@michael-small
Copy link
Contributor Author

@rainerhahnekamp

I am marking this as ready since it is basically done and covers most scenarios.

  • I think a lot of this is probably overkill or repeated/redundant test code, but it seems to works as is. All the methods tested, with or without a named collection, for its basic state patching as well as loading state.
  • I am a bit stuck on testing the error flow as I have not had any success with mocking it out. I tried either spying on the simple new Store() and mock implementing it throwing an error, or making a new StoreWithErrors that throw errors in the implementations, and other variations to no success. I am curious what you would suggest, or if you could please show me an example of how you might write one.

Perhaps this is too undercooked a submission as well but I think I may have overthought/overdone certain aspects and would appreciate feedback before going further in a fruitless direction.

Copy link
Collaborator

@rainerhahnekamp rainerhahnekamp left a comment

Choose a reason for hiding this comment

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

Good work so far. I'd suggest that in the first round of review, we focus on slimming down the amount of code. Depending on where we end up, we could then maybe split the file into multiple files or everything is alright.

TestBed.runInInjectionContext(() => {
const store = new Store();

tick(1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

tick() should be sufficient. That applies to all your tests.

updateAll(entity: Flight[]): Promise<Flight[]> {
return firstValueFrom(
of([
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd create a factory function which generates you a flight. That function should use by a default an default flight object and increments the id automatically with every call.

Its signature could be createFlight(flight: Partial<Flight>): Flight. As you see, you then only need to provide those values of Flight which differ from the default one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should slim down all these mocking helpers, good idea.

@michael-small
Copy link
Contributor Author

Good work so far. I'd suggest that in the first round of review, we focus on slimming down the amount of code. Depending on where we end up, we could then maybe split the file into multiple files or everything is alright.

Sounds good, thanks. I'll slim it down with your suggestions and a couple things that have occurred to me since and then @ you when it's at that point.

PR comment:

"tick() should be sufficient. That applies to all your tests."

Change:

Most uses of `tick(...)` were replaced, but ones that were
time-sensitive to the mocks for loading time were retained
PR comment:

I'd create a factory function which generates you a flight. That function should use by a default an default flight object and increments the id automatically with every call.

Its signature could be createFlight(flight: Partial<Flight>): Flight. As you see, you then only need to provide those values of Flight which differ from the default one.

Change:

Made same function.
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