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(scan): implement scan functionality #43

Merged
merged 102 commits into from Apr 29, 2022

Conversation

RomanReznichenko
Copy link
Contributor

closes #42

@codeclimate
Copy link

codeclimate bot commented Apr 20, 2022

Code Climate has analyzed commit 0305650 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1
Duplication 2

The test coverage on the diff in this pull request is 99.3% (50% is the threshold).

This pull request will bring the total coverage in the repository to 97.9% (1.0% change).

View more on Code Climate.

Copy link
Member

@derevnjuk derevnjuk left a comment

Choose a reason for hiding this comment

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

There are multiple bugs, please address them first, then I will proceed with the review.

In addition, pay attention to these comments as well:

  • rename the requests folder to commands (we don't have such an entity)
  • remove the useless enums folder (the folder structure should reflect the domain)
  • for each bug that I mention you should write a dedicated test case

packages/scan/project.json Outdated Show resolved Hide resolved
packages/scan/package.json Outdated Show resolved Hide resolved
packages/scan/src/utils/delay.ts Outdated Show resolved Hide resolved
packages/scan/tsconfig.lib.json Outdated Show resolved Hide resolved
packages/scan/tsconfig.spec.json Outdated Show resolved Hide resolved
packages/scan/src/ScanFactory .ts Outdated Show resolved Hide resolved
packages/scan/src/HarEntryBuilder.ts Outdated Show resolved Hide resolved
packages/scan/src/ScanFactory .ts Outdated Show resolved Hide resolved
packages/scan/src/Scan.ts Outdated Show resolved Hide resolved
packages/scan/src/Scan.ts Outdated Show resolved Hide resolved
@derevnjuk
Copy link
Member

@RomanReznichenko please resolve codeclimate issues first

Copy link
Member

@derevnjuk derevnjuk left a comment

Choose a reason for hiding this comment

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

There are still multiple issues and bugs:

  • The file is not satisfied HAR spec 1.2, e.g. postData is expected to be an object, cookies are empty, etc
  • Wrong query params serialization
  • Wrong default body serialization in the HarEntryBuilder
  • No error handling in the Scans
  • Scan stops even while getting 502 error
  • etc

@RomanReznichenko leave everything as is, I will fix them soon

packages/bus/src/index.ts Outdated Show resolved Hide resolved
packages/scan/README.md Outdated Show resolved Hide resolved
packages/scan/src/DefaultScans.spec.ts Outdated Show resolved Hide resolved
packages/scan/project.json Outdated Show resolved Hide resolved
packages/scan/src/DefaultScans.spec.ts Outdated Show resolved Hide resolved
packages/scan/src/DefaultScans.spec.ts Outdated Show resolved Hide resolved
packages/scan/src/DefaultScans.ts Outdated Show resolved Hide resolved
packages/scan/src/DefaultScans.ts Outdated Show resolved Hide resolved
packages/scan/src/DefaultScans.ts Outdated Show resolved Hide resolved
packages/scan/src/DefaultScans.ts Outdated Show resolved Hide resolved
@derevnjuk derevnjuk force-pushed the feat/scan branch 4 times, most recently from 6d14f6d to b13452f Compare April 28, 2022 13:05
@derevnjuk derevnjuk force-pushed the feat/scan branch 3 times, most recently from c36ac30 to 5eedcdc Compare April 28, 2022 23:55
@derevnjuk derevnjuk requested review from pmstss and removed request for pmstss April 29, 2022 00:00
pmstss
pmstss previously approved these changes Apr 29, 2022
packages/core/src/utils/contains.spec.ts Outdated Show resolved Hide resolved
packages/scan/README.md Outdated Show resolved Hide resolved
packages/scan/README.md Outdated Show resolved Hide resolved
packages/scan/src/Scan.spec.ts Outdated Show resolved Hide resolved
pmstss
pmstss previously approved these changes Apr 29, 2022
Copy link
Contributor

@pmstss pmstss left a comment

Choose a reason for hiding this comment

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

please check expectation/expectations wording in package description

pmstss
pmstss previously approved these changes Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement scan module
3 participants