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

feat(docs,emmet-expressjs): add e2e api specification #29

Conversation

thiagomini
Copy link
Contributor

Closes #26

Hey @oskardudycz , I haven't added the ES db yet because I'm unsure how to replace the inMemoryStore(). However, I believe this draft may suffice as an initial discussion for that issue. It took me some time to figure out how everything worked and glued together, but I feel I'm understanding the architecture better now 😅

One thing I'd like to ask / discuss is the no-unsafe-assignement rule that we're using here and it's complaining all over the place - is this intended? Honestly, in many cases it doesn't make sense, look at this example:

image

The getInMemoryEventStore() function has a defined return value, yet this rule is complaining 🤔

Anyway, let me know what you think about this initial implementation!

let shoppingCartId: string;

beforeEach(() => {
clientId = randomUUID();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice that we don't need the uuid() library to generate uuid v4 - it's already supported by node's native "crypto" module

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, cool. For those needs, this will be sufficient. Happy to remove the dependency on uuid package!

.send(productItem),
)
.when((request) =>
request.get(`/clients/${clientId}/shopping-carts/current`).send(),
Copy link
Contributor Author

@thiagomini thiagomini Mar 2, 2024

Choose a reason for hiding this comment

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

I've created this temporary workaround get method - I thought that it would be good to do this and in the future we just need to change the underlying implementation detail to use projections

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, sounds good to me, I like the idea. Thanks to that, we'll have the full coverage.

@@ -52,6 +52,7 @@ export enum ShoppingCartStatus {
Pending = 'Pending',
Confirmed = 'Confirmed',
Canceled = 'Canceled',
Opened = 'Opened',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was necessary so I could check for it in the e2e test

@@ -44,7 +44,6 @@ describe('Application logic with optimistic concurrency', () => {

if (!current.id) {
assert.fail();
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

when we call assert.fail(), node automatically throws an assertion error, so this line was unreachable

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, didn't know that. Thanks for the hint!

@@ -59,9 +61,9 @@ export const expectNewEvents = <EventType extends Event = Event>(
};

export const expectResponse =
(
<Body = unknown>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A small addition to the expectResponse matcher so we can optionally type the body

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes perfect sense to me, thanks for doing that! 🫡

clientId: assertNotEmptyString(request.params.clientId),
id: shoppingCartId,
productItems,
status: result?.state.status,
Copy link
Collaborator

Choose a reason for hiding this comment

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

SUGGESTION: I think that instead of using ? here, you could return NotFound status if result is null

@@ -139,6 +141,40 @@ export const shoppingCartApi =
return NoContent();
}),
);

// Get Shopping Cart
router.get(
Copy link
Collaborator

Choose a reason for hiding this comment

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

FIX: @thiagomini, for now I'd prefer to keep the sample outside of the complete-api region as the example in the getting started is (for now) focused on business logic and writing. I'm planning to introduce the read model side as the next step. Would you mind moving this endpoint outside the region?

@@ -139,6 +141,40 @@ export const shoppingCartApi =
return NoContent();
}),
);

// Get Shopping Cart
router.get(
Copy link
Collaborator

Choose a reason for hiding this comment

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

HINT: The method as a test method is okay as it is. You could consider providing the customised evolve method that would already build the desired read model without the need to do the additional mapping. But that's a nice to have if you wanted to play more with event sourcing. This would be a dedicated ShoppingCartDetails type with is own evolve function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only change I didn't apply, @oskardudycz - I'd like to dedicate a separate issue to that, so I can focus on creating that projection

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fine by me 🙂

@@ -151,6 +161,50 @@ export const ApiSpecification = {
},
};

export const ApiE2ESpecification = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

CONCERN: @thiagomini, maybe we could break those specifications into separate files? Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can do that!

@@ -151,6 +161,50 @@ export const ApiSpecification = {
},
};

export const ApiE2ESpecification = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, and it looks good to me, just as I imagined it 👍

- Updated http responses to allow returning problem without options (e.g. NotFound)
- Fixed path to ESDB package in tsconfig
@oskardudycz oskardudycz marked this pull request as ready for review March 4, 2024 16:00
@oskardudycz
Copy link
Collaborator

Regarding the lint errors, in my case for VSCode restarting the ESLint server fixes issue (and it's a "known issue" on their side, see https://discord.com/channels/1211601942984532018/1211601943433060433/1213043016512573480 :|). For ts, it's not but might be that's mishap on my side configuring paths. I'll try to look into that.

Copy link
Collaborator

@oskardudycz oskardudycz left a comment

Choose a reason for hiding this comment

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

@thiagomini it looks good to me even as it is. Check my comments, if you'd like to apply some of them. I added the EventStoreDB config for e2e tests, see details in my commit 🙂

- Move ApiE2ESpecification to its own file
- Move common test setup functions to utils file
- Fixed typos "succeded" -> "succeeded"
- Changed api to return not found when shopping cart does not exist
…hiagomini/emmett into feat/add-request-based-arrange-section
} else if (Array.isArray(verify)) {
const [first, ...rest] = verify;

if (typeof first === 'function') {
const succeded = first(response);
const succeeded = first(response);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@thiagomini, thanks for fixing my typos! 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, no problem haha I only got them because I have the "Code Spell Cheker" extension in my VSCode

@oskardudycz oskardudycz added enhancement New feature or request testing WebApi labels Mar 5, 2024
@oskardudycz oskardudycz added this to the 0.4.0 milestone Mar 5, 2024
@oskardudycz oskardudycz merged commit d649d83 into event-driven-io:main Mar 5, 2024
1 check passed
mkubasz pushed a commit to mkubasz/emmett that referenced this pull request Mar 5, 2024
…o#29)

* feat(docs,emmet-expressjs): add e2e api specification

closes event-driven-io#26

* Configured E2E tests for using ESDB

- Updated http responses to allow returning problem without options (e.g. NotFound)
- Fixed path to ESDB package in tsconfig

* refactor(docs,emmett-expressjs): apply review changes

- Move ApiE2ESpecification to its own file
- Move common test setup functions to utils file
- Fixed typos "succeded" -> "succeeded"
- Changed api to return not found when shopping cart does not exist

* fix(emmett-expressjs): fix import statement

---------

Co-authored-by: Oskar Dudycz <oskar.dudycz@gmail.com>
oskardudycz added a commit that referenced this pull request Mar 6, 2024
* feat: prepare base fastify template

feat: add some defaults plugins

feat: start working on tests

feat: working with types

feat: finished test

feat: finished test

feat: update package

* feat: general server options

* Fix martendb.io link in README.md

* feat(docs,emmet-expressjs): add e2e api specification (#29)

* feat(docs,emmet-expressjs): add e2e api specification

closes #26

* Configured E2E tests for using ESDB

- Updated http responses to allow returning problem without options (e.g. NotFound)
- Fixed path to ESDB package in tsconfig

* refactor(docs,emmett-expressjs): apply review changes

- Move ApiE2ESpecification to its own file
- Move common test setup functions to utils file
- Fixed typos "succeded" -> "succeeded"
- Changed api to return not found when shopping cart does not exist

* fix(emmett-expressjs): fix import statement

---------

Co-authored-by: Oskar Dudycz <oskar.dudycz@gmail.com>

* fix: remove default middlewares to extend developers expirience

* Fixed fastify API tests and typing around plugins

Fixed also linter errors

---------

Co-authored-by: futpib <futpib@gmail.com>
Co-authored-by: Thiago Valentim <51245643+thiagomini@users.noreply.github.com>
Co-authored-by: Oskar Dudycz <oskar.dudycz@gmail.com>
@oskardudycz oskardudycz modified the milestones: 0.4.0, 0.5.0 Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request testing WebApi
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to setup integration tests through HTTP requests
2 participants