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.

Sign up for GitHub

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

Jump to bottom

test: change folder structure #712

Merged
merged 9 commits into from
Sep 12, 2023
Merged

test: change folder structure #712

merged 9 commits into from
Sep 12, 2023

Conversation

Shurtu-gal
Copy link
Collaborator

@Shurtu-gal Shurtu-gal commented Aug 2, 2023 •
edited
Loading

Description

  • I have made this PR considering that I also have to add unit and integration tests.
  • Before, as there were only functionality tests, I put them in the functionality folder.
  • These refactoring will help in adding new tests under other categories.

Related issue(s)
#599

@Shurtu-gal Shurtu-gal changed the title test: increase test-coverage test: change folder structure Aug 2, 2023
@Shurtu-gal Shurtu-gal marked this pull request as ready for review August 2, 2023 09:31
Souvikns
Souvikns reviewed Aug 2, 2023
View reviewed changes
Copy link
Member

@Souvikns Souvikns left a comment

Choose a reason for hiding this comment

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

I see you have moved the tests to a different folder, can you update the pr comment and talk about why you are making this change and how it will benefit us?

@Shurtu-gal
Copy link
Collaborator Author

@Souvikns, I have updated the comment.

@Shurtu-gal Shurtu-gal requested a review from Souvikns August 2, 2023 11:00
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Shurtu-gal
Copy link
Collaborator Author

@Souvikns @fmvilas @derberg I have to decide whether I should keep using jest for testing purposes or use mocha for it. I would love your opinions.

Reasons for not using mocha :- #353

Reasons for shifting to mocha:-

  • It has better support for oclif. Hence gives more accurate coverage

With Mocha

image

With Jest

image

Some alternatives can be to mock the commands as functions and test them instead of running them as cli. But I don't have a viable approach for doing so ( Would love your guidance out here 😮‍💨 )

@chinma-yyy
Copy link
Contributor

The issue is with the code transpiling. I think we need not shift to mocha instead we can change the collect coverage parameter to live/**/*.js which will collect coverage from the compiled code which will give us the accurate result. Would love to hear on this more. I tried changing the transformer and some config didn't find something useful

@Shurtu-gal
Copy link
Collaborator Author

@chinma-yyy Let me also try then 🤖

Souvikns
Souvikns approved these changes Sep 5, 2023
View reviewed changes
Copy link
Member

@Souvikns Souvikns left a comment

Choose a reason for hiding this comment

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

@Shurtu-gal wait for @derberg's approval before merging.

derberg
derberg approved these changes Sep 11, 2023
View reviewed changes
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

LGTM
after merging just please go through remaining 10PRs and add comment to the ones that add tests, with info what people have to do to update and follow latest master

@Shurtu-gal
Copy link
Collaborator Author

LGTM
after merging just please go through remaining 10PRs and add comment to the ones that add tests, with info what people have to do to update and follow latest master

👍

@derberg Volta.net
Copy link
Member

derberg commented Sep 12, 2023

@Souvikns @Shurtu-gal I guess it can be merged? Feel free to do so when ready

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Shurtu-gal
Copy link
Collaborator Author

/rtm

@asyncapi-bot asyncapi-bot merged commit 385842d into asyncapi:master Sep 12, 2023
11 checks passed
This was referenced Sep 12, 2023
Merged
Closed
Closed
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.55.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

peter-rr
peter-rr reviewed Sep 18, 2023
View reviewed changes
Copy link
Member

@peter-rr peter-rr left a comment

Choose a reason for hiding this comment

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

LGTM!

@Shurtu-gal Shurtu-gal deleted the feat/test-coverage branch September 19, 2023 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peter-rr peter-rr peter-rr left review comments

@Souvikns Souvikns Souvikns approved these changes

@derberg derberg derberg approved these changes

@magicmatatjahu magicmatatjahu Awaiting requested review from magicmatatjahu

@asyncapi-bot-eve asyncapi-bot-eve Awaiting requested review from asyncapi-bot-eve asyncapi-bot-eve is a code owner

Assignees
No one assigned
Labels
ready-to-merge released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Shurtu-gal @chinma-yyy @derberg @asyncapi-bot @Souvikns @peter-rr