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

fix: handle skipped test correctly #24

Merged
merged 1 commit into from
Nov 27, 2019
Merged

fix: handle skipped test correctly #24

merged 1 commit into from
Nov 27, 2019

Conversation

gearsdigital
Copy link
Contributor

@gearsdigital gearsdigital commented Nov 24, 2019 •
edited
Loading

This PR fixes #23.

I slightly changed the regular expression to match skipped test cases and suites too.

  1. describe.skip
  2. describe.only
  3. fdescribe
  4. xdescribe
  5. it.skip
  6. it.only
  7. fit
  8. xit

This regex will match things like describe.lorem or describeLore() too. I guess this shouldn't be a problem because Jasmine or Mocha won't execute it either.

You can see it in action here: https://regex101.com/r/HUyh3u/1 and find a more detailed explanation here: #23 (comment)

@gearsdigital gearsdigital mentioned this pull request Nov 24, 2019
Open
@coveralls
Copy link

coveralls commented Nov 24, 2019 •
edited
Loading

Pull Request Test Coverage Report for Build 140

  • 14 of 14 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 98.595%
Totals Coverage Status
Change from base Build 139: 0.03%
Covered Lines: 736
Relevant Lines: 738
💛 - Coveralls

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build 140

  • 14 of 14 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 98.595%
Totals Coverage Status
Change from base Build 139: 0.03%
Covered Lines: 736
Relevant Lines: 738
💛 - Coveralls

@gearsdigital gearsdigital changed the title fix(path-finder): handle skipped test correctly fix: handle skipped test correctly Nov 24, 2019
@fadc80
Copy link
Owner

fadc80 commented Nov 27, 2019 •
edited
Loading

First of all, thank you for opening this PR.

I was able to reproduce the error using this repo:

https://github.com/gearsdigital/karma-sonarqube-reporter-issue-23

You really got it. The regex doesn't match mocha skipped tests:

  • describe.skip | describe.only
  • it.skip | it.only

By other hand, the original regex can deal with jasmine skipped tests:

  • xdescribe | fdescribe
  • xit | fit

Since not matching the initials x and f doesn't affect the final result.

Additionally, I downloaded your fix and I used it in your sample project.

I noticed it's still not working as expected.

This was the result produced:

START:
  helper
    ✔ should transform a string to uppercase letters
    ✖ should be skipped (skipped)
ERROR: Please, report issue => Test file path not found! {"src/test/helper.spec.js":{"describe":["helper"],"it":["should transform a string to uppercase letters"]}} | helper | should be skipped

Finished in 0.011 secs / 0 secs @ 23:12:53 GMT-0300 (Brasilia Standard Time)

SUMMARY:
✔ 1 test completed
ℹ 1 test skipped

Steps to reproduce:

# Create a folder
mkdir patches && cd patches
# Clone both repositories
git clone https://github.com/gearsdigital/karma-sonarqube-reporter.git 
git clone https://github.com/gearsdigital/karma-sonarqube-reporter-issue-23.git
# Navigate to your fix
cd karma-sonarqube-reporter
# Install your fix
npm install
# Navigate to your sample project
cd ../karma-sonarqube-reporter-issue-23
# Remove karma-sonarqube-reporter dependency
sed -i '/"karma-sonarqube-reporter": "^1.2.5"/d' ./package.json
# Remove comment from skipped test
sed -i 's/\/\///g' ./src/test/helper.spec.js
# Install your sample project
npm install
# Add a link to your fix
npm link ../karma-sonarqube-reporter
# Run your tests
npm run test

To solve this problem you can do two changes:

  1. Replace your regex by this one: https://regex101.com/r/vStcpw/1

  2. Adjust the following code snipped:

karma-sonarqube-reporter/lib/path-finder.js

Lines 20 to 35 in 25d76db

function parseTestFile(paths, path, data) {
var result, regex = regexPattern();
while ((result = regex.exec(data)) != null) {
var type = result[2] || result[3];
var text = result[5];
if (paths[path] == undefined) {
paths[path] = { describe: [], it: [] };
}
if (type === 'describe') {
paths[path].describe.push(text);
}
if (type === 'it') {
paths[path].it.push(text);
}
}
}

Make theses changes:

Line 28: replace type === 'describe' by type.startsWith('describe')
Line 31: replace type === 'it' by type.startsWith('it')

Could you do that, please?

Thank you!

@gearsdigital
Copy link
Contributor Author

gearsdigital commented Nov 27, 2019 •
edited
Loading

@fad I really appreciate your feedback but I think you forgot to checkout the branch which contains the fix. Your are testing against your master.

# Create a folder
mkdir patches && cd patches
# Clone both repositories
git clone https://github.com/gearsdigital/karma-sonarqube-reporter.git 
git clone https://github.com/gearsdigital/karma-sonarqube-reporter-issue-23.git
# Navigate to your fix
cd karma-sonarqube-reporter
# Install your fix
npm install
# Checkout branch containing the fix
git checkout fix-skipped-specs
# Navigate to your sample project
cd ../karma-sonarqube-reporter-issue-23
# Remove karma-sonarqube-reporter dependency
sed -i '/"karma-sonarqube-reporter": "^1.2.5"/d' ./package.json
# Remove comment from skipped test
sed -i 's/\/\///g' ./src/test/helper.spec.js
# Install your sample project
npm install
# Add a link to your fix
npm link ../karma-sonarqube-reporter
# Run your tests
npm run test

@fadc80
Copy link
Owner

fadc80 commented Nov 27, 2019 •
edited
Loading

Yes, I've made a mistake. Sorry about that.

@fadc80
Copy link
Owner

fadc80 commented Nov 27, 2019

LGTM

@fadc80 fadc80 merged commit 3f9e3b2 into fadc80:master Nov 27, 2019
@gearsdigital
Copy link
Contributor Author

Awesome! Thank you very much for merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ERROR: Please, report issue => Test file path not found! {} |
3 participants
@gearsdigital @coveralls @fadc80