chore: Use correct config file to run lint on test files and refactor test code #165

adebayor123 · 2022-05-23T22:47:05Z

Issue

Previously, we did not run lint on test files. After the addition of #144 and #149 , we are running lint on staged files and the input for --project was tsconfig.webpack.json. Even though tsconfig.webpack.json extended from tsconfig.json which defined project as all the files under src/, tsconfig.webpack.json overwrote the project to only include dependencies involved with index-browser.ts.

As a result, any changes/addition of test files would fail in commit sessions, as it was recognized as "not included in the project".

Solution

There were two ways to go about this issue: (1) do not include test files (non-source code) under pre-commit (2) use tsconfig.json as input for --project and refactor our test cases.

Despite being test files, applying lint to test cases in the future and doing the refactoring will increase code quality - thus went with option (2).

This PR

This PR makes two major changes:

instead of tsconfig.webpack.json, use tsconfig.json for npm run lint
refactor existing test cases

Notes

Since TSLint is deprecated as of 2018, we should migrate to ESLint. There will be a subsequent PR to migrate from TSLint to ESLint.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

… test code

maniator · 2022-05-24T02:57:11Z

src/plugins/event-plugins/__integ__/DomEventPlugin.test.ts

-    for (let i = 0; i < events.length; i++) {
-        let eventType = events[i].type;
-        let eventDetails = JSON.parse(events[i].details);
+    for (const event of events) {


This not not so safe to do with arrays

Consider using Array.forEach

Hey, could you explain why this isn't a safe operation with arrays? This was recommended by TSLint and I'm not able to find much information on why forEach is preferred - although according to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of, it has compatibility issues with IE, so I will go ahead and make the change.

I forget the reasons 100%, but I believe in some cases (and some browsers) for..of returns the length value as well.

maniator · 2022-05-24T02:57:54Z

src/plugins/event-plugins/__tests__/DomEventPlugin.test.ts

-        for (let i = 0; i < record.length; i++) {
-            expect(record.mock.calls[i][0]).toEqual(DOM_EVENT_TYPE);
-            expect(record.mock.calls[i][1]).toMatchObject(
+        for (const call of record.mock.calls) {


See above

Please use forEach

maniator · 2022-05-24T03:01:50Z

src/test-utils/smoke-test-utils.ts

@@ -85,8 +87,8 @@ export const isEachEventIngested = async (
    // Running tests in parallel require pagination logic, as several test cases have the same timestamp
    while (true) {
        const data = await rumClient.send(command);
-        for (let i = 0; i < data.Events.length; i++) {
-            ingestedEvents.add(JSON.parse(data.Events[i]).event_id);
+        for (const event of data.Events) {


Same as all other comments related to for of

qhanam · 2022-05-24T04:01:50Z

src/event-cache/__tests__/EventCache.test.ts

@@ -12,7 +12,7 @@ const getSession = jest.fn(() => ({
    eventCount: 1
 }));
 const getUserId = jest.fn(() => 'b');
-const getAttributes = jest.fn(() => {});
+const getAttributes = jest.fn(() => undefined);


nit: I think the intent with {} (here and elsewhere) was to return an empty object.

Clearly it's not being used though... so could even just do

const getAttributes = jest.fn();

or omit getAttributes.

() => {} does not return an empty object, it returns undefined

In order to return an empty object one has to do: () => ({}) (note the extra parenthesis)

Yep, apparently {} leads to undefined (and unit tests prove the case to be so as well since all were passing).
I will do the following:

first try out using () => ({}) to create an empty object as maniator@ suggests, since that was our original intention.

remove unused references.

adebayor123 · 2022-05-24T16:56:18Z

TODO: before merging, rename commit to be less than 50 characters

adebayor123 · 2022-05-24T17:14:08Z

src/plugins/event-plugins/__integ__/DomEventPlugin.test.ts

-    for (let i = 0; i < events.length; i++) {
-        let eventType = events[i].type;
-        let eventDetails = JSON.parse(events[i].details);
+    for (const event of events) {


TODO: use forEach

adebayor123 · 2022-05-24T17:14:16Z

src/plugins/event-plugins/__integ__/DomEventPlugin.test.ts

-    for (let i = 0; i < events.length; i++) {
-        let eventType = events[i].type;
-        let eventDetails = JSON.parse(events[i].details);
+    for (const event of events) {


TODO: use forEach

adebayor123 · 2022-05-24T17:14:21Z

src/plugins/event-plugins/__integ__/DomEventPlugin.test.ts

-    for (let i = 0; i < events.length; i++) {
-        let eventType = events[i].type;
-        let eventDetails = JSON.parse(events[i].details);
+    for (const event of events) {


TODO: use forEach

adebayor123 · 2022-05-24T17:14:26Z

src/plugins/event-plugins/__integ__/DomEventPluginMutationObserverEnabled.test.ts

-    for (let i = 0; i < events.length; i++) {
-        let eventType = events[i].type;
-        let eventDetails = JSON.parse(events[i].details);
+    for (const event of events) {


TODO: use forEach

adebayor123 · 2022-05-24T17:14:32Z

src/plugins/event-plugins/__integ__/DomEventPluginMutationObserverEnabled.test.ts

-    for (let i = 0; i < events.length; i++) {
-        let eventType = events[i].type;
-        let eventDetails = JSON.parse(events[i].details);
+    for (const event of events) {


TODO: use forEach

adebayor123 · 2022-05-24T17:14:52Z

src/plugins/event-plugins/__tests__/DomEventPlugin.test.ts

-        for (let i = 0; i < record.mock.calls.length; i++) {
-            expect(record.mock.calls[i][0]).toEqual(DOM_EVENT_TYPE);
-            expect(record.mock.calls[i][1]).toMatchObject(
+        for (const call of record.mock.calls) {


TODO: use forEach

adebayor123 · 2022-05-24T17:15:13Z

src/plugins/event-plugins/__tests__/DomEventPlugin.test.ts

-        for (let i = 0; i < record.length; i++) {
-            expect(record.mock.calls[i][0]).toEqual(DOM_EVENT_TYPE);
-            expect(record.mock.calls[i][1]).toMatchObject(
+        for (const call of record.mock.calls) {


TODO: use forEach

adebayor123 · 2022-05-24T17:15:52Z

src/test-utils/smoke-test-utils.ts

-    for (let i = 0; i < eventIds.length; i++) {
-        if (!ingestedEvents.has(eventIds[i])) {
-            throw new Error(`Event ${eventIds[i]} not ingested.`);
+    for (const eventId of eventIds) {


TODO: use forEach

chore: Use correct config file to run lint on test files and refactor…

f113449

… test code

adebayor123 requested review from limhjgrace and qhanam May 23, 2022 22:47

maniator reviewed May 24, 2022

View reviewed changes

qhanam reviewed May 24, 2022

View reviewed changes

qhanam approved these changes May 24, 2022

View reviewed changes

adebayor123 commented May 24, 2022

View reviewed changes

adebayor123 added 2 commits May 24, 2022 12:27

fix: Use forEach instead of for..of

19ed7af

fix: Create empty object instead of undefined

5a09534

adebayor123 merged commit ae60f2a into aws-observability:main May 24, 2022

adebayor123 deleted the fix-lint branch May 24, 2022 20:01

qhanam mentioned this pull request May 31, 2022

DOM event plugin add data-rum-id attribute #163

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Use correct config file to run lint on test files and refactor test code #165

chore: Use correct config file to run lint on test files and refactor test code #165

adebayor123 commented May 23, 2022

maniator May 24, 2022

adebayor123 May 24, 2022

maniator May 24, 2022

maniator May 24, 2022

maniator May 24, 2022

qhanam May 24, 2022

maniator May 24, 2022

adebayor123 May 24, 2022

adebayor123 commented May 24, 2022

adebayor123 May 24, 2022

adebayor123 May 24, 2022

adebayor123 May 24, 2022

adebayor123 May 24, 2022

adebayor123 May 24, 2022

adebayor123 May 24, 2022

adebayor123 May 24, 2022

adebayor123 May 24, 2022

chore: Use correct config file to run lint on test files and refactor test code #165

chore: Use correct config file to run lint on test files and refactor test code #165

Conversation

adebayor123 commented May 23, 2022

Issue

Solution

This PR

Notes

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

adebayor123 commented May 24, 2022

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