-
-
Notifications
You must be signed in to change notification settings - Fork 97
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
docs: explanation on how to deal with tests failing on windows #575
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
README.md
Outdated
1. Make sure code is well formatted and secure `npm run lint` | ||
2. Make sure all tests pass `npm test` | ||
|
||
If in-case some tests still fails randomly during local development, the reason to it might be when you develop something in the parser and even though you did not make any changes affects tests, and the tests are failing then you can run: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is only relevant for windows systems, I think we should highlight that somehow here, what do you think @Ruchip16🤔?
Maybe it makes sense to add a sub section such as ### Windows environments
and place this information there? 🤔
cc @alequetzalli in case you have time and have an opinion 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we can add that also then we need to add sub-sections for Mac or Linux users as well then right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not unless you have something specific you want to add there 🙂 To me a section only makes sense if there is something to highlight, i.e. in this case windows environment might act weird in some cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, would do the changes in the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonaslagoni Totally, if it's just for Windows
OS, let's def add a ### Windows
title.
(I don't think you need to write the word 'environment' too, it's pretty clear to just say Windows.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to change this sentence I think: the reason to it might be when you develop something in the parser and even though you did not make any changes affects tests
To me it is not clear what exactly the reason is. Maybe something along the lines of:
If in-case some tests still fails randomly during local development, the reason to it might be when you develop something in the parser and even though you did not make any changes affects tests, and the tests are failing then you can run: | |
For Windows environments, some tests might still fail randomly during local development even when you made no changes to the tests. The reason for this from file endings are different than expected and this comes from Git defaulting to an unexpected file ending. If you encounter this issue you can run the following commands to set Git to use the expected one: |
What do you think @Ruchip16 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yess absolutely the later one gives more clarity than its prior, I would do the changes
@Ruchip16 hey, when you will work on updates to this PR, please also adjust the title of this PR, to follow conventional commits. In other words, change it into something like |
hey, yes I would do it within 1-2 days as I was busy with the campus placement drives, but no worries I'm back, I would fix it asap. |
Kudos, SonarCloud Quality Gate passed! |
README.md
Outdated
@@ -235,9 +235,17 @@ A few advantages of this solution: | |||
|
|||
## Develop | |||
|
|||
### Windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move this section below the already existing list of steps to do during development, and then the line about the windows specific scenario you place this section in the end.
The reason is that this list of steps is not only related to windows. It is only the special case about the file ending 🙂
README.md
Outdated
1. Make sure code is well formatted and secure `npm run lint` | ||
2. Make sure all tests pass `npm test` | ||
|
||
If in-case some tests still fails randomly during local development, the reason to it might be when you develop something in the parser and even though you did not make any changes affects tests, and the tests are failing then you can run: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to change this sentence I think: the reason to it might be when you develop something in the parser and even though you did not make any changes affects tests
To me it is not clear what exactly the reason is. Maybe something along the lines of:
If in-case some tests still fails randomly during local development, the reason to it might be when you develop something in the parser and even though you did not make any changes affects tests, and the tests are failing then you can run: | |
For Windows environments, some tests might still fail randomly during local development even when you made no changes to the tests. The reason for this from file endings are different than expected and this comes from Git defaulting to an unexpected file ending. If you encounter this issue you can run the following commands to set Git to use the expected one: |
What do you think @Ruchip16 ?
@Ruchip16 Any update? Do you have a time to apply a review suggestions? |
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @derberg do you wanna something?
Kudos, SonarCloud Quality Gate passed! |
This pull request has been automatically marked as stale because it has not had recent activity 😴 It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation. There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model. Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here. Thank you for your patience ❤️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for such a HUGE delay @Ruchip16, thought this was taken care of. My apologies 🙇
Kudos, SonarCloud Quality Gate passed! |
/rtm |
not an issue, glad that it is getting merged now, can you also add me to contributors? |
@all-contributors please add @Ruchip16 for docs |
I've put up a pull request to add @Ruchip16! 🎉 |
🎉 This PR is included in version 2.1.0-next-major-spec.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 2.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Related issue(s)