Add more tests and/or lint

[phil-davis]

We had this problem today that replaceAll was used in some code. But replaceAll is not supported until nodejs 15 or later. See PR #76 for some details.

It would be nice to know in CI if new code uses some nodejs feature that does not exist in the nodejs version being used in the docker image. I tried running the lint with nodejs 14, but lint still passed - see PR #77

Maybe there is some other code analysis tool that will quickly tell us about node version compatibility problems?

Or we make more unit and/or integration tests that will actually execute each piece of code. That should cause fails when executed with a nodejs version that is too old for the code.

For discussion...

[phil-davis]

CC @kiranparajuli589 @Talank @SwikritiT @SwikritiT @individual-it

[individual-it]

I think we should add more tests, that should not be too hard. If there are situations we cannot tests with unit tests, even adding e2e API tests would be OK here

[kiranparajuli589]

Findings so far:

Did some research on this, find out that eslint does not care of this kind of issue. See eslint/eslint#16176 (comment)

But there is an eslint plugin called eslint-plugin-n which has some implementations to detect such cases. See eslint-community/eslint-plugin-n#43 (comment)

One another approach can be the use of typeScript where we can configure the node base as https://www.typescriptlang.org/docs/handbook/tsconfig-json.html#tsconfig-bases.

[saw-jan]

In PR #128, I have made the node and eslint parser versions strict so that the incompatible versions and codes are reported. With this, we can get early feedback in ci as well as locally.

[kiranparajuli589]

@saw-jan can we detect issues like the following now (with yours PR) with eslint?

We had this problem today that replaceAll was used in some code. But replaceAll is not supported until NodeJS 15 or later.

[saw-jan]

Yeah, 99% sure 😄
node >16.3 supports 100% of ecma2021 features + some ecma2022 features.
And if a feature only available from ecma2022 is used then eslint should report that

For example:
we can do the following with node >16.3

class Test {
  static data = "some value"
}

But eslint will report that as assignment to static member is the feature available from ecma2022

  2:15  error  Parsing error: Unexpected token =
✖ 1 problem (1 error, 0 warnings)

[saw-jan]

With PR #128, there is a restriction to the js features we can use in the code and the eslint will report that.
Also, the incompatible version of node will be reported (which was the main concern of this issue)

As per the tests, we could think of any better way. The problem with replaceAll came from the stepDefs code and that code only runs when there is /execute request with the provided step. If we are to test this then we might need to create API test for each step available, but I have no idea how it will benefit us.

[kiranparajuli589]

Yes, for the code in stepDefs, I think adding API acceptance tests would be good.

[saw-jan]

We will need to have a thorough discussion on how to add those API tests and also discuss the feasibility and benefits of those tests.

CC @kiranparajuli589 @phil-davis @individual-it

[individual-it]

I think adding more API tests for this is over engineering. Let's close this for now as a lot of issues should be caught by the linter