Write events test suite #181

shravanhariharan2 · 2021-04-24T04:43:44Z

Closes #175

github-actions · 2021-04-24T04:43:56Z

Thanks for contributing! If you've made changes to the API's functionality, please make sure to bump the package version—see this guide to semantic versioning for details—and document those changes as appropriate.

sumeet-bansal

Looks good overall. I think we should mock StorageService instead of actually writing/reading to an S3 bucket (and definitely not to the same folder that prod uses), and we should start passing in our configs to constructors to make it easier to mock things.

sumeet-bansal · 2021-05-03T16:59:54Z

services/EventService.ts

@@ -18,7 +18,7 @@ export default class EventService {
  public async create(event: Event) {
    const eventCreated = await this.transactions.readWrite(async (txn) => {
      const eventRepository = Repositories.event(txn);
-      const isUnusedAttendanceCode = eventRepository.isUnusedAttendanceCode(event.attendanceCode);
+      const isUnusedAttendanceCode = await eventRepository.isUnusedAttendanceCode(event.attendanceCode);


Did you catch this with a test or just notice it?

That was caught with test - I saw that the duplicate attendance code error was thrown by Postgres' duplicate key constraint and not by the event service

Nice, our first bug caught by testing!

sumeet-bansal · 2021-05-03T17:03:16Z

tests/data/EventFactory.ts

+    return EventFactory.fakeWithTime(start, end);
+  }
+
+  private static fakeWithTime(start: Date, end: Date): EventModel {


You can use the with method instead of fakeWithTime, e.g.

private static fakeWithTime(start: Date, end: Date): EventModel { return with({ start, end }); }

Totally forgot about with() - we actually don't even need fakeWithTime or fakeWithoutTime anymore if with() is used for fakePastEvent(), fakeOngoingEvent(), and fakeFutureEvent().

sumeet-bansal · 2021-05-03T17:03:54Z

tests/data/EventFactory.ts

+    return event;
+  }
+
+  public static fakeWithoutTime(): EventModel {


What's the different between this and fake? Or does this method exist because of fakeWithTime?

As mentioned above, this method is not needed anymore

But the idea before was to have fake, fakePastEvent, fakeOngoingEvent, fakeFutureEvent to all call fakeWithTime, but specifying different times for each section. fakeWithTime would then call fakeWithoutTime to get a random object for all other fields

sumeet-bansal · 2021-05-03T17:08:57Z

tests/data/EventFactory.ts

+    return [start.toDate(), end.toDate()];
+  }
+
+  private static randomPastTime(daysAgo: number): [Date, Date] {


Can we reuse code between these randomTime methods? Maybe we have optional params where we can set some days, hour, and duration fields?

sumeet-bansal · 2021-05-03T17:10:02Z

tests/data/FeedbackFactory.ts

@@ -19,6 +19,10 @@ export class FeedbackFactory {
    };
  }

+  public static createEventFeedback(n: number): string[] {
+    return new Array(n).fill(faker.random.word());


Should this be a random word or some value from a predefined array of values? IIRC feedback's supposed to be some predefined emojis.

I'm not sure the predefined emojis have been confirmed yet since a lot of the design materials are moving around right now, and membership hasn't acknowledged them either since the portal redesign has been pushed back significantly (till after the store frontend is finished). I think it's fine to keep them at random words for now and then adjust later once the set of emojis /text is confirmed.

sumeet-bansal · 2021-05-03T17:28:36Z

tests/event.test.ts

+      .createEvents([event])
+      .write();
+
+    const eventCoverResponse = await ControllerFactory.event(conn).updateEventCover(cover, event.uuid, admin);


I don't think our tests should write to our prod bucket. At least, definitely not to the same folder. This is what I said a while back, that we should pass in our configs through the constructor, so we can specify where things get written to instead of being tied to whatever's in the prod config.

I'm not too sure what we should pass in to the StorageService constructor, since I believe most of our config (such as bucket to be written to) is unchangeable. The S3_BUCKET, S3_REGION, and S3_CREDENTIALS are all constant through the entire app, and there's separate buckets for local (acmucsd-local, for what is used for locally running the test suite and in CircleCI jobs), testing (acmucsd-membership-portal, for testing.api.acmucsd.com), and prod (acmucsd, for what is used in production), so tests don't actually get written to our prod bucket.

sumeet-bansal · 2021-05-03T17:30:17Z

tests/data/FileFactory.ts

+const ONE_BYTE_FILLER = 'a';
+
+// Factory for mocking in-memory files for testing with multer
+export class FileFactory {


If we do test that we're writing/reading from S3 should we also verify the file is exactly the same? If so, maybe we write a random string and verify checksums? I don't think we need to actually interact with S3 though, mocking should be good enough.

Right, mocking should be good enough.

sumeet-bansal · 2021-05-03T17:34:26Z

tests/event.test.ts

+    await ControllerFactory.event(conn).submitEventFeedback(event.uuid, { feedback }, user);
+
+    const attendanceResponse = await ControllerFactory.attendance(conn).getAttendancesForCurrentUser(user);
+    const attendance = attendanceResponse.attendances[0];


Nit: inline.

tests/event.test.ts

sumeet-bansal · 2021-05-03T17:36:46Z

tests/event.test.ts

+      .write();
+
+    const eventController = ControllerFactory.event(conn);
+    const errorMessage = 'Attendance code has already been used';


Nit: inline this and the other error messages.

michl1001

Looks good overall though I'm not as intimately familiar with our test suite (whether there are better functions to use) and the higher-level design

Co-authored-by: Sumeet Bansal <[email protected]>

shravanhariharan2 · 2021-05-18T23:57:30Z

@sumeet-bansal how do you think we should go about mocking StorageService and passing in config into the constructor? To my understanding we can't just wrap that service like we do with controllers since the service would still upload to S3 and I don't think we wanted that, and I'm still not really sure what we'd pass in to the constructor of StorageService since all the config is the same except the bucket, which is changed within the environment variables.

sumeet-bansal · 2021-05-19T16:58:09Z

What do you mean by wrapping controllers? Jest should have some sort of mocking functionality where we can do something like

const storageService = mock(IStorageService.class);
when(storageService.upload(any(), any(), eq(MediaType.EVENT_COVER), eq("some-uuid")).thenReturn("fileLocation");

(That's the syntax for the Mockito library in Java.)

But the idea is, we create a "mock" and then we can define custom responses based on the parameters, and instead of executing normally, the mock returns our defined response.

sumeet-bansal · 2021-05-19T17:00:41Z

This looks like what we need (link).

sumeet-bansal · 2021-05-22T02:28:17Z

Mock StorageService not the S3 library.

shravanhariharan2 · 2021-05-22T04:25:30Z

Wait, why mock StorageService? Isn't that something we'd want to actually test, but just not have anything uploaded? My thinking was that we don't want anything uploaded, so we just replace the this.s3.upload call with a different function that doesn't upload anything, but that still returns the URL that would be returned by aws-sdk.

shravanhariharan2 · 2021-05-22T04:27:24Z

I might just be generally confused of the concept/use case of mocking cause I think I've been the word throwing around here and there with the controllers without knowing if that's what we're actually doing with them.

sumeet-bansal · 2021-05-22T04:44:21Z

No you're right, we could mock aws-sdk. S3Mock looks more difficult than it needs to be but maybe that's just because of jest. There isn't much of a difference between mocking aws-sdk and StorageService functionality-wise (we wouldn't verify the upload path but I don't think that's a big deal), so I figured it'd be easier to just mock StorageService instead.

shravanhariharan2 · 2021-05-26T23:15:18Z

S3Mock looks more difficult than it needs to be but maybe that's just because of jest.

Yeah it's like that since that's how jest does mocking for modules. If we were to mock StorageService instead, then all we can assert is just that event.cover exists right? Instead of being able to check that event covers are uploaded to the event path instead of banner path, etc. (since that was also a bug that happened earlier).

sumeet-bansal · 2021-05-27T00:24:13Z

Yikes, maybe we should look into a better library for mocking. Looks like there's a TypeScript version of Mockito: https://github.com/NagRock/ts-mockito!
We could separately test the static method that takes an enum and returns the upload info. Normally I'd say let's mock aws-sdk but StorageService is just a wrapper and the only thing we'd be testing is a really simple switch statement.

…e optional before but implicitly)

shravanhariharan2 · 2021-05-27T20:27:41Z

services/EventService.ts

      if (changes.attendanceCode !== currentEvent.attendanceCode) {
-        const isUnusedAttendanceCode = eventRepository.isUnusedAttendanceCode(changes.attendanceCode);
+        const isUnusedAttendanceCode = await eventRepository.isUnusedAttendanceCode(changes.attendanceCode);
        if (!isUnusedAttendanceCode) throw new UserError('Attendance code has already been used');


sumeet-bansal · 2021-05-28T00:17:04Z

package.json

@@ -56,6 +56,7 @@
    "pg": "^8.6.0",
    "reflect-metadata": "^0.1.13",
    "routing-controllers": "^0.9.0",
+    "ts-mockito": "^2.6.1",


Nit: dev dependency.

sumeet-bansal · 2021-05-30T22:19:43Z

tests/controllers/index.ts

+import { Connection } from 'typeorm';
+import FeedbackService from '../../services/FeedbackService';
+import { FeedbackController } from '../../api/controllers/FeedbackController';
+import { UserController } from '../../api/controllers/UserController';
+import UserAccountService from '../../services/UserAccountService';
+import StorageService from '../../services/StorageService';
+import { AdminController } from '../../api/controllers/AdminController';
+import AttendanceService from '../../services/AttendanceService';
+import { AttendanceController } from '../../api/controllers/AttendanceController';
+import { AuthController } from '../../api/controllers/AuthController';
+import { EventController } from '../../api/controllers/EventController';
+import { LeaderboardController } from '../../api/controllers/LeaderboardController';
+import { MerchStoreController } from '../../api/controllers/MerchStoreController';
+import UserAuthService from '../../services/UserAuthService';
+import EmailService from '../../services/EmailService';
+import EventService from '../../services/EventService';
+import MerchStoreService from '../../services/MerchStoreService';


Nit: can we clean these imports up a bit in a follow-up PR? Let's import by api/controllers and services.

Sure, just created an issue for it

sumeet-bansal · 2021-05-30T22:19:54Z

tests/controllers/index.ts

+import EventService from '../../services/EventService';
+import MerchStoreService from '../../services/MerchStoreService';
+
+export class ControllerFactory {


This class is getting messy. Let's remove the controller singletons and initialize a new controller each time the factory's called.

True. Is the reason we are willing to trade-off readability for memory because this class is used only in tests and isn't persisting any memory afterwards? (meaning we are okay using a small amount more of memory since it's all present only during the tests running)

sumeet-bansal · 2021-05-30T22:23:46Z

tests/controllers/index.ts

+import EventService from '../../services/EventService';
+import MerchStoreService from '../../services/MerchStoreService';
+
+export class ControllerFactory {


Nit: rename file to the class name.

sumeet-bansal · 2021-05-30T22:26:33Z

api/validators/FeedbackControllerRequests.ts

@@ -24,11 +23,6 @@ export class Feedback implements IFeedback {
  type: FeedbackType;
 }

-export class SubmitEventFeedbackRequest implements ISubmitEventFeedbackRequest {


What's this change for?

Moved SubmitEventFeedbackRequest from FeedbackControllerRequests to EventControllerRequests

sumeet-bansal · 2021-05-30T22:36:52Z

tests/event.test.ts

+    const attendanceResponse = await ControllerFactory.attendance(conn).getAttendancesForCurrentUser(user);
+
+    expect(attendanceResponse.attendances[0].feedback).toEqual(feedback);
+    expect(user.points).toEqual(event.pointValue + Config.pointReward.EVENT_FEEDBACK_POINT_REWARD);


How is this passing? Shouldn't we need to refresh the UserModel before the points field is updated?

https://github.com/acmucsd/membership-portal/blob/master/repositories/UserRepository.ts#L52

This line updates the points field directly on the user object, so no refresh is necessary.

sumeet-bansal · 2021-05-30T22:38:56Z

tests/event.test.ts

+    expect(errors).toBeDefined();
+    expect(errors).toHaveLength(1);
+    expect(errors[0].property).toEqual('feedback');


Can we add an expect that checks that the reason validation failed is because >3?

sumeet-bansal · 2021-05-30T22:39:44Z

tests/feedback.test.ts

-    const uuid = submittedFeedbackResponse.feedback;
-    const acknowledgedFeedback = await feedbackController.updateFeedbackStatus(uuid, { status }, admin);
+    const { uuid } = submittedFeedbackResponse.feedback;
+    const acknowledgedFeedback = await feedbackController.updateFeedbackStatus({ uuid }, { status }, admin);


I don't see any changes to the feedback controller so why this change?

I believe it was a missing change from this PR that wasn't causing the test to fail for some reason.

sumeet-bansal · 2021-05-30T22:43:31Z

tests/mocks/index.ts

+import StorageService from '../../services/StorageService';
+
+export default class Mocks {
+  public static storage(fileLocation: string) {


Inline this and use verify to check that we called the mock correctly.

I'm not too sure what you mean by this. What should we be inlining, and where should we be verifying the call?

sumeet-bansal · 2021-05-30T22:45:03Z

tests/event.test.ts

+      .event(conn, null, Mocks.storage(fileLocation))
+      .updateEventCover(cover, { uuid: event.uuid }, admin);
+
+    expect(eventCoverResponse.event.cover).toEqual(fileLocation);


I think we should get the event and check the cover from that instead of checking the updateEventCoverResponse.

Code from #181 adds ability to fake events in the past, future or present. Taken verbatim from commit 18a952c.

* Add tests for "/bonus" and "/emails" routes Squashed set of commits for 6f51db8...c70e444. Merge conflict was on package-lock, which was hard to fix, so I just redid the commits. * nit: "proxyUser" -> "adminUser" for admin tests * Adjust array equality checks on admin tests * Remove test for non-present emails Test is not necessary due to lack of filtering in original controller. * Inline functions and variables in admin tests * Modify test to check bonus points for all users * Combine both bonus points tests into one Tests had basically the same boilerplate. Combine them to check both conditions at once. The test already checked if we were adding bonus point to _specifically_ the emails we passed in, so it's best to also check if an extra user wasn't included at the same time. * Rearrange asserts to suffix response calls * Rename adminUser to admin in admin test suite * Adjust order of assertions in admin test * Adjust name of extraneous user in admin test * Make variable for UserController in admin test Inlining UserController made one line too long. Since it is used multiple times in the test, extract variable to make its usage less verbose. * Fix nits and check attendance response details * Check for correct activities in admin test suite * Add points field of ActivityModel in test suite ActivityModel is missing the `pointsEarned` field whenever creating an Activity after attending an event. This only happens in our mock tester. Add a check for this field's presence, along with fixing the inherent bug within the mock state. * Port EventFactory code from #181 Code from #181 adds ability to fake events in the past, future or present. Taken verbatim from commit 18a952c. * Adjust test suite to account for factory bug Account creation can occur after event attendance, causing activities to be out of order chronologically. Guarantee order in activities by introducing retroactive attendance in a future event. Bug in factory is not a reproducible issue in production, since account creation will always occur before event attendance in real time. * Revert event factory code changes * Adjust transaction order when saving factory state Mock factory `write` function incorrectly saves attendance before any possible activities, typically causing event attendances to be stored in the database prior to activities being stored. Adjust order of flushing to database in order to consistently set smaller timestamps for account-related activities, such as account creation. * Revert transaction order adjustment This reverts commit b5f03dc. Tests continue to be non-deterministic despite this change. My assumption was wrong. * Guarantee correct activity order in failed tests Create users at beginning of the Unix epoch to ensure guarantee of event attendance activity being second is fulfilled. Since Activities are sorted by timestamp, we need to guarantee account creation does not occur before attendance of event. Fix issue with above workaround, providing explicit method whenever necessary. * Merge user creation methods to have optional args Rather than having two explicit methods, merge methods and use optional timestamp argument to allow for custom creation of users at explicit times. * Adjust user creation method to have set timestamp Use specified timestamp of 1 month before current time to place account creation in time before most activities for testing. * Fix argument call for creating users in test suite Co-authored-by: Matei-Alexandru Gardus <[email protected]>

shravanhariharan2 · 2021-09-18T15:54:10Z

Closing this for now since this will not be worked on for quite a bit, as merch store issues are being worked on for the October launch. I'll reopen this once things have settled with the store.

This branch should not be deleted

* Add tests for "/bonus" and "/emails" routes Squashed set of commits for 6f51db8...c70e444. Merge conflict was on package-lock, which was hard to fix, so I just redid the commits. * nit: "proxyUser" -> "adminUser" for admin tests * Adjust array equality checks on admin tests * Remove test for non-present emails Test is not necessary due to lack of filtering in original controller. * Inline functions and variables in admin tests * Modify test to check bonus points for all users * Combine both bonus points tests into one Tests had basically the same boilerplate. Combine them to check both conditions at once. The test already checked if we were adding bonus point to _specifically_ the emails we passed in, so it's best to also check if an extra user wasn't included at the same time. * Rearrange asserts to suffix response calls * Rename adminUser to admin in admin test suite * Adjust order of assertions in admin test * Adjust name of extraneous user in admin test * Make variable for UserController in admin test Inlining UserController made one line too long. Since it is used multiple times in the test, extract variable to make its usage less verbose. * Fix nits and check attendance response details * Check for correct activities in admin test suite * Add points field of ActivityModel in test suite ActivityModel is missing the `pointsEarned` field whenever creating an Activity after attending an event. This only happens in our mock tester. Add a check for this field's presence, along with fixing the inherent bug within the mock state. * Port EventFactory code from acmucsd#181 Code from acmucsd#181 adds ability to fake events in the past, future or present. Taken verbatim from commit 18a952c. * Adjust test suite to account for factory bug Account creation can occur after event attendance, causing activities to be out of order chronologically. Guarantee order in activities by introducing retroactive attendance in a future event. Bug in factory is not a reproducible issue in production, since account creation will always occur before event attendance in real time. * Revert event factory code changes * Adjust transaction order when saving factory state Mock factory `write` function incorrectly saves attendance before any possible activities, typically causing event attendances to be stored in the database prior to activities being stored. Adjust order of flushing to database in order to consistently set smaller timestamps for account-related activities, such as account creation. * Revert transaction order adjustment This reverts commit b5f03dc. Tests continue to be non-deterministic despite this change. My assumption was wrong. * Guarantee correct activity order in failed tests Create users at beginning of the Unix epoch to ensure guarantee of event attendance activity being second is fulfilled. Since Activities are sorted by timestamp, we need to guarantee account creation does not occur before attendance of event. Fix issue with above workaround, providing explicit method whenever necessary. * Merge user creation methods to have optional args Rather than having two explicit methods, merge methods and use optional timestamp argument to allow for custom creation of users at explicit times. * Adjust user creation method to have set timestamp Use specified timestamp of 1 month before current time to place account creation in time before most activities for testing. * Fix argument call for creating users in test suite Co-authored-by: Matei-Alexandru Gardus <[email protected]>

shravanhariharan2 added 9 commits April 17, 2021 21:33

Add skeleton event tests

c12ac20

Add missing await to EventService::createEvent

ef89fa3

Add rest of CRUD operation tests

bc43ff9

Add storage utils for mocking images to upload

ce6dcb8

Add event feedback fakes

290c828

Add empty file check for storage test utils

159d169

Refactor EventFactory to fake past, ongoing, and future events easily

bbaedc2

Remove unused validator in feedback

6a9b5ae

Finish writing preliminary event tests

74db6ba

shravanhariharan2 requested a review from sumeet-bansal as a code owner April 24, 2021 04:43

Merge branch 'master' into tests/events

d2fd439

shravanhariharan2 requested a review from michl1001 April 24, 2021 04:44

sumeet-bansal requested changes May 3, 2021

View reviewed changes

michl1001 approved these changes May 4, 2021

View reviewed changes

shravanhariharan2 and others added 8 commits May 4, 2021 11:23

Update tests/event.test.ts

8d7623d

Co-authored-by: Sumeet Bansal <[email protected]>

Refactor EventFactory fake methods

a264273

Inline all error messages that are only used once

caa4f92

Fix small event test nits

a33bcd0

Merge branch 'master' into tests/events

fcc0e4a

Change controller params in test to match updated validation format

e56e157

Move asserts next to data fetches

cc6f83b

Refactor EventFactory random time generation

8ef3ea1

sumeet-bansal mentioned this pull request May 7, 2021

Bump underscore from 1.10.2 to 1.12.1 #183

Closed

shravanhariharan2 mentioned this pull request May 18, 2021

Add AdminController Test Suite #187

Merged

shravanhariharan2 added 2 commits May 18, 2021 16:48

Merge branch 'master' into tests/events

5b3fa0c

Switch to non-deprecated hexadecimal generation

6e55d48

mocking with jest sucks

2f8c3b4

shravanhariharan2 added 2 commits May 27, 2021 13:26

Add 'update event' tests

2529a86

Make event patches fields explicitly optional in validation (they wer…

28a55ca

…e optional before but implicitly)

shravanhariharan2 commented May 27, 2021

View reviewed changes

shravanhariharan2 added 3 commits May 27, 2021 14:32

Allow mocked services to be injected into controllers

ede38bf

Remove redundant type

539d35e

Switch mocking aws-sdk to StorageService using ts-mockito

18a952c

shravanhariharan2 requested a review from sumeet-bansal May 27, 2021 21:37

sumeet-bansal requested changes May 30, 2021

View reviewed changes

StormFireFox1 pushed a commit that referenced this pull request Jul 17, 2021

Port EventFactory code from #181

747b44d

Code from #181 adds ability to fake events in the past, future or present. Taken verbatim from commit 18a952c.

shravanhariharan2 added 9 commits July 17, 2021 13:05

Merge branch 'master' into tests/events

342dcb5

Move ts-mockito from dep to devDep

a70a68e

Remove singleton behavior from ControllerFactory

cc1694b

Move controllers/index.ts -> ControllerFactory.ts

9b51a89

Move createEventFeedback from FeedbackFactory to EventFactory

3ecb774

Move fakeOngoingEvent start duration forward 30 minutes

2c5ca2a

Tighten error handling to match constraint in test

a88706e

Lint

98023da

Add get call to event cover test to ensure cover is persisted

298f5f3

shravanhariharan2 requested a review from sumeet-bansal July 21, 2021 05:17

shravanhariharan2 closed this Sep 18, 2021

Write events test suite #181

Write events test suite #181

Conversation

shravanhariharan2 commented Apr 24, 2021

github-actions bot commented Apr 24, 2021

sumeet-bansal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michl1001 left a comment

Choose a reason for hiding this comment

shravanhariharan2 commented May 18, 2021

sumeet-bansal commented May 19, 2021

sumeet-bansal commented May 19, 2021

sumeet-bansal commented May 22, 2021

shravanhariharan2 commented May 22, 2021

shravanhariharan2 commented May 22, 2021

sumeet-bansal commented May 22, 2021

shravanhariharan2 commented May 26, 2021

sumeet-bansal commented May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shravanhariharan2 commented Sep 18, 2021 • edited Loading

sumeet-bansal commented May 27, 2021 •

edited

Loading

shravanhariharan2 commented Sep 18, 2021 •

edited

Loading