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

mocha.path option should be used to find the tests #127

Merged
merged 2 commits into from Mar 26, 2019

Conversation

eramitmittal
Copy link
Contributor

@eramitmittal eramitmittal commented Jul 16, 2018

mocha.path option should be treated as relative to the workspace root folder


This change is Reviewable

…the tests

mocha.path option should be treated as relative to the workspace root folder
@maty21
Copy link
Owner

maty21 commented Jul 16, 2018

I'm not sure sometimes you wish to use global mocha.
did you saw any problem with this?

@maty21
Copy link
Owner

maty21 commented Jul 16, 2018

btw on which configuration you run sidebar for typescript there is an open issue about running mocha sidebar with typescript. but I haven't looked at it yet

@eramitmittal
Copy link
Contributor Author

Hi,
I am not aware about the mocha sidebar issue with typescript. I will try and have a look.
The problem I faced with my project setup is:

  1. My project uses mocha version 5.2.0
  2. Sidebar extension ships with mocha version 2.5.3
  3. If I specify full path to local mocha installation using mocha.path option, sidebar extension loads the correct version
  4. If I specify relative path, sidebar extension always loads the mocha version shipped with the extension
  5. I want to specify relative path so that I can commit this in version control. My team members in this way will not be required to manually specify the full path as per their machine.

Regarding your concern that sometimes people want to use global mocha, I can modify the code to use path.isAbsolute to check if the configuration really is relative or an absolute path

Consider mocha.oath config relative to workspace only if a relative path has been specified
@eramitmittal
Copy link
Contributor Author

I have updated the code, please review

@eramitmittal
Copy link
Contributor Author

hi, did you get a chance to review this (adoption of sidebar extension in my team is blocked on this issue, I hope you understand :))

@eramitmittal
Copy link
Contributor Author

Hi, did you get a chance to review this?

@maty21
Copy link
Owner

maty21 commented Jul 24, 2018 via email

@eramitmittal
Copy link
Contributor Author

eramitmittal commented Jul 25, 2018

Yes, without this change I am not able to specify relative path to 'local' mocha installation.
Ability to specify relative path is important so that the setting can be committed to version control.
Otherwise I have to use 'absolute' path which is off-course different on different machines. Due to this each and every dev in my team will be required to manually specify the path. This is undesirable.

My patch is just 'completing' the functionality of mocha.path setting.
Since mocha.path already allows specification of absolute path, it is only natural that it should also support relative path.

@eramitmittal
Copy link
Contributor Author

Interestingly, documentation for mocha.path suggests that user can provide relative path.
But it does not work.
My patch is solving this by treating a relative path (if specified) w.r.t. workspace root folder

@eramitmittal
Copy link
Contributor Author

Hi @maty21 , Any update on this please?

@maty21
Copy link
Owner

maty21 commented Dec 21, 2018 via email

Copy link
Owner

@maty21 maty21 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@maty21 maty21 merged commit 5879627 into maty21:master Mar 26, 2019
maty21 added a commit that referenced this pull request Mar 26, 2019
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.

None yet

2 participants