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

refactor(tests): API tests #32643

Merged
merged 19 commits into from
Jun 24, 2024
Merged

refactor(tests): API tests #32643

merged 19 commits into from
Jun 24, 2024

Conversation

tassoevan
Copy link
Contributor

@tassoevan tassoevan commented Jun 20, 2024

Proposed changes (including videos or screenshots)

It migrates integration tests to TypeScript.

Issue(s)

Steps to test or reproduce

Further comments

The goal of this changes is to raise the maintenance rate and, subsequently, the quality of these tests.

Before

🟠 JavaScript files: 333
🔵 TypeScript files: 6465
      🧪 Conversion: 95.10%

🟠 JavaScript LOCs: 55515
🔵 TypeScript LOCs: 409232
      🧪 Conversion: 88.05%

  JavaScript biggest files:
    apps/meteor/tests/end-to-end/api/01-users.js (125.04kb)
    apps/meteor/server/models/raw/Users.js (56.83kb)
    apps/meteor/app/slackbridge/server/SlackAdapter.js (43.78kb)
    apps/meteor/app/markdown/lib/hljs.js (19.49kb)
    packages/node-poplib/src/index.js (18.83kb)
    packages/eslint-config/style/index.js (15.81kb)
    packages/eslint-config/imports/index.js (10.80kb)
    packages/eslint-config/best-practices/index.js (9.15kb)
    apps/meteor/public/enc.js (2.99kb)
    packages/message-parser/webpack.config.js (1.79kb)

After


🟠 JavaScript files: 292
🔵 TypeScript files: 6500
      🧪 Conversion: 95.70%

🟠 JavaScript LOCs: 30317
🔵 TypeScript LOCs: 434714
      🧪 Conversion: 93.48%

  JavaScript biggest files:
    apps/meteor/server/models/raw/Users.js (56.83kb)
    apps/meteor/app/slackbridge/server/SlackAdapter.js (43.78kb)
    apps/meteor/app/markdown/lib/hljs.js (19.49kb)
    packages/node-poplib/src/index.js (18.83kb)
    packages/eslint-config/style/index.js (15.81kb)
    packages/eslint-config/imports/index.js (10.80kb)
    packages/eslint-config/best-practices/index.js (9.15kb)
    apps/meteor/public/enc.js (2.99kb)
    packages/message-parser/webpack.config.js (1.79kb)
    packages/eslint-config/react.js (0.97kb)

Copy link
Contributor

dionisio-bot bot commented Jun 20, 2024

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is targeting the wrong base branch. It should target 6.11.0, but it targets 6.10.0

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

Copy link

changeset-bot bot commented Jun 20, 2024

⚠️ No Changeset found

Latest commit: 191e534

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

codecov bot commented Jun 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.72%. Comparing base (1827e95) to head (b654f09).
Report is 3 commits behind head on develop.

Current head b654f09 differs from pull request most recent head 191e534

Please upload reports for the commit 191e534 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #32643      +/-   ##
===========================================
- Coverage    56.73%   56.72%   -0.01%     
===========================================
  Files         2496     2496              
  Lines        55360    55359       -1     
  Branches     11455    11455              
===========================================
- Hits         31407    31403       -4     
+ Misses       21255    21253       -2     
- Partials      2698     2703       +5     
Flag Coverage Δ
e2e 56.47% <ø> (-0.01%) ⬇️
unit 71.85% <100.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@tassoevan tassoevan force-pushed the refactor/api-tests branch 6 times, most recently from 8f8cb55 to a7a6016 Compare June 22, 2024 20:09
apps/meteor/tests/data/users.helper.ts Dismissed Show dismissed Hide dismissed
@tassoevan tassoevan force-pushed the refactor/api-tests branch 2 times, most recently from b654f09 to 191e534 Compare June 23, 2024 12:29
@tassoevan tassoevan added this to the 6.10 milestone Jun 24, 2024
@tassoevan tassoevan marked this pull request as ready for review June 24, 2024 15:43
@tassoevan tassoevan requested review from a team as code owners June 24, 2024 15:43
@tassoevan tassoevan modified the milestones: 6.10, 7.0 Jun 24, 2024
expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('files').and.to.be.an('array');
})
.end(done);
});

it('should not return thumbnails', async function () {
it('should not return thumbnails', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

are these changes because of our eslint rules? just want to point this out https://mochajs.org/#arrow-functions

I wonder if should have a mix of arrow and non-arrow functions when the this context is needed, or if we should stick to one or the other

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ESLint rules. However, @typescript-eslint ensures functions are converted to arrow functions only when there is no references to this.
Personally, it feels like there is very little situations where we need to use the Mocha context e.g. this.retries(n) was redudant with the current configuration and calls to this.skip() are rare and easily replaced with it.skip/describe.skip.

@sampaiodiego
Copy link
Member

do you have a before/after of JS/TS files? I'm just curious about the new stats

@tassoevan tassoevan modified the milestones: 7.0, 6.11 Jun 24, 2024
@ggazzo ggazzo added the stat: QA assured Means it has been tested and approved by a company insider label Jun 24, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jun 24, 2024
@ggazzo ggazzo merged commit e07a515 into develop Jun 24, 2024
60 of 77 checks passed
@ggazzo ggazzo deleted the refactor/api-tests branch June 24, 2024 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants