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

Removed testing dependency #5

Merged
merged 5 commits into from
Sep 25, 2024
Merged

Removed testing dependency #5

merged 5 commits into from
Sep 25, 2024

Conversation

abroa01
Copy link
Collaborator

@abroa01 abroa01 commented Sep 21, 2024

Removed testing dependency as this package has been tested locally using npm link.

Copy link
Member

@horner horner left a comment

Choose a reason for hiding this comment

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

I would leave the testing dependency, but change the test so it returns passed.

@wreiske
Copy link
Member

wreiske commented Sep 21, 2024

What is your reasoning for removing it? We don't want to publish broken code to NPM. Ensuring the test passes successfully before pushing to NPM is a critical step in ci/cd

Copy link
Member

@wreiske wreiske left a comment

Choose a reason for hiding this comment

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

We should not remove testing from CI / CD.

@horner
Copy link
Member

horner commented Sep 22, 2024

The test requires a webchart system. I asked him to remove the dependency on webchart. I think the test should do something that can be done completely independent and later will simulate a WebChart system.

@abroa01
Copy link
Collaborator Author

abroa01 commented Sep 23, 2024

Hi Will,

  • Removed test cases with WebChart dependencies and sensitive info (username, password, practice).
  • Used sinon to mock functionality, avoiding external system calls.
  • Added basic test cases to validate core functionality (initialization, session management, method definitions).

@wreiske
Copy link
Member

wreiske commented Sep 23, 2024

Awesome! Can you do me one last favor? It would be good to have the pull request run the npm test, that way we can make sure the tests are passing before merging into the main branch. You can add a second workflow file named "check-pull-request" and set the on to push.

https://medium.com/marionete/using-github-actions-to-help-review-pull-requests-using-unit-tests-and-code-coverage-72bde49ed3a2

updated the .yml file to install the dependency to run npm test
@abroa01
Copy link
Collaborator Author

abroa01 commented Sep 23, 2024

Hey Will,

I have added a GitHub actions to test the Pull request and run all the tests before merging it.

Can you please check if that fine or any change is needed.

Adding Test cases for ApiService
@horner horner merged commit 380f998 into main Sep 25, 2024
1 check passed
@horner horner linked an issue Sep 25, 2024 that may be closed by this pull request
@abroa01 abroa01 deleted the bug/fixingTestingIssue branch October 3, 2024 22:35
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.

Make an npm from this repo
3 participants