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

MarkdownBear: Add test to check message for error #1280

Merged
merged 1 commit into from
Jan 13, 2017

Conversation

namanyadav12
Copy link
Contributor

@namanyadav12 namanyadav12 commented Jan 10, 2017

A better test for MarkdownBear to check the exact message
of the result for a maximum line length error.

Related to #1235

@gitmate-bot
Copy link
Collaborator

Thanks for your contribution!

Reviewing pull requests take really a lot of time and we're all volunteers. Please make sure you go through the following check list and complete them all before pinging someone for a review.

As you learn things over your Pull Request please help others on the chat and on PRs to get their stuff right as well!

with execute_bear(self.uut, fname, file) as results:
self.assertEqual(results[0].message,
'Line must be at most 10 characters'
' maximum-line-length remark-lint')
Copy link
Member

Choose a reason for hiding this comment

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

hm the spacing is a bit weird, why is this ' maximum-line-length remark-lint' included in the message anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup,I actually improved the output_regex and the removed the 'maximum-line-length remark-lint' in #1277 but that pr is on hold till this issue is resolved.

@sils
Copy link
Member

sils commented Jan 10, 2017

since you haven't actually done a bugfix but rather prove that there is no bug I'd use closes, not fixes. Also commit message should sum up the change, let's try something like MarkdownBear: Add test for message or so.

The code change looks rather good and we have to rethink where our docker image bug could be... :/

@sils
Copy link
Member

sils commented Jan 10, 2017

👍

@gitmate-bot
Copy link
Collaborator

Comment on 7abd34b.

Shortlog of the HEAD commit contains 65 character(s). This is 15 character(s) longer than the limit (65 > 50).

GitCommitBear, severity NORMAL, section commit.

@fneu
Copy link

fneu commented Jan 11, 2017

ack f1a0a25

@fneu
Copy link

fneu commented Jan 11, 2017

@rultor merge

@rultor
Copy link

rultor commented Jan 11, 2017

@rultor merge

@fneu OK, I'll try to merge now. You can check the progress of the merge here

@rultor
Copy link

rultor commented Jan 11, 2017

@rultor merge

@fneu @namanyadav12 Oops, I failed. You can see the full log here (spent 1min)

+ git remote update
Fetching origin
Fetching fork
From github.com:namanyadav12/coala-bears
 * [new branch]      Makman2/crosstest -> fork/Makman2/crosstest
 * [new branch]      Makman2/docproto -> fork/Makman2/docproto
 * [new branch]      Makman2/postcss -> fork/Makman2/postcss
 * [new branch]      Makman2/testing-crosstest -> fork/Makman2/testing-crosstest
 * [new branch]      Makman2/tmp -> fork/Makman2/tmp
 * [new branch]      MarkdownBear_linelength -> fork/MarkdownBear_linelength
 * [new branch]      abhsag/linebreak -> fork/abhsag/linebreak
 * [new branch]      fixannot   -> fork/fixannot
 * [new branch]      hypothesist/wip -> fork/hypothesist/wip
 * [new branch]      markdown_fix -> fork/markdown_fix
 * [new branch]      markdown_remark-validate-links -> fork/markdown_remark-validate-links
 * [new branch]      master     -> fork/master
 * [new branch]      red/extend-upload-tool -> fork/red/extend-upload-tool
 * [new branch]      release/0.1 -> fork/release/0.1
 * [new branch]      release/0.2 -> fork/release/0.2
 * [new branch]      release/0.7 -> fork/release/0.7
 * [new branch]      release/0.8 -> fork/release/0.8
 * [new branch]      release/0.9 -> fork/release/0.9
 * [new branch]      sanketdg/docextration/capitalizebear -> fork/sanketdg/docextration/capitalizebear
 * [new branch]      sanketdg/docformatbear -> fork/sanketdg/docformatbear
 * [new branch]      sils/bfx   -> fork/sils/bfx
 * [new branch]      sils/infer -> fork/sils/infer
 * [new branch]      sils/lc    -> fork/sils/lc
 * [new branch]      sils/linters -> fork/sils/linters
 * [new branch]      sils/pmd   -> fork/sils/pmd
 * [new branch]      sils/quotes -> fork/sils/quotes
 * [new branch]      sils/sec   -> fork/sils/sec
 * [new branch]      sils/tailor -> fork/sils/tailor
 * [new branch]      sils/tailorbackport -> fork/sils/tailorbackport
 * [new branch]      sils/upgcoala -> fork/sils/upgcoala
 * [new branch]      sils/yapf-old -> fork/sils/yapf-old
 * [new branch]      underyx/go-caching -> fork/underyx/go-caching
 * [new branch]      underyx/req-version-specs -> fork/underyx/req-version-specs
 * [new branch]      xml_style  -> fork/xml_style
+ args=
+ '[' only == default ']'
+ '[' only == no ']'
+ '[' only == only ']'
+ args=' --ff-only'
+ export BRANCH=__rultor
+ BRANCH=__rultor
++ git show-branch __rultor
++ wc -l
+ '[' 0 -gt 0 ']'
+ git checkout -B __rultor fork/markdown_fix
Switched to a new branch '__rultor'
Branch __rultor set up to track remote branch markdown_fix from fork.
+ git checkout -B master origin/master
Switched to and reset branch 'master'
Branch master set up to track remote branch master from origin.
+ '[' false == true ']'
+ '[' false == true ']'
+ git merge --ff-only __rultor
fatal: Not possible to fast-forward, aborting.
'cid' file is absent, container wasn't started correctly

@fneu
Copy link

fneu commented Jan 11, 2017

@namanyadav12 please rebase on lastest master, thanks :)

@@ -16,7 +16,7 @@
"jshint": "~2",
"postcss-cli": "~2",
"remark-cli": "~2",
"remark-lint": ">=5.1.0",
"remark-lint": "~5.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

Is this change related?
Is the problem that 5.4.0 (the latest) was being installed, and that version is broken??
If yes ... that needs to be clearly stated in the commit message.
If these tests work with 5.4.0, then changing this file should be a separate PR, as it is not related.

(Note that due to caching, it isnt obvious which versions are installed in the logs, unless the build is without the cache)

@jayvdb
Copy link
Member

jayvdb commented Jan 12, 2017

unack 2e2ce17

Copy link
Member

@jayvdb jayvdb left a comment

Choose a reason for hiding this comment

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

I do not see any information on the issue, or PR, which suggests that this PR can fix the bug.
Adding tests obviously can not fix a bug.
And there is no analysis of the versions of remark-lint which might explain why altering the package.json will fix the bug (it might, but I cant see any analysis).

@namanyadav12 namanyadav12 changed the title MarkdownBear: Fix MarkdownBear MarkdownBear: Add test to check message for error Jan 12, 2017
@namanyadav12
Copy link
Contributor Author

namanyadav12 commented Jan 12, 2017

@jayvdb the commit message is already modified. I changed it earlier when sils asked me to.
The title of the pr is however the same. I will change that also.

@namanyadav12
Copy link
Contributor Author

Also changed Closes to Related to

@jayvdb
Copy link
Member

jayvdb commented Jan 12, 2017

I think you have two spaces after "Related to ", and you still include the change to "package.json".
(git checkout HEAD~1 package.json to remove that, then commit and push)

@jayvdb
Copy link
Member

jayvdb commented Jan 13, 2017

ack 99ca12e

@jayvdb
Copy link
Member

jayvdb commented Jan 13, 2017

https://circleci.com/gh/coala/coala-bears/4882 has passed on CircleCI, but Github is still waiting for it to finish :/

I'm going to restart it

@jayvdb
Copy link
Member

jayvdb commented Jan 13, 2017

needs rebase

A better test for MarkdownBear to check the exact message
of the result for a maximum line length error.

Related to coala#1235
@aptrishu
Copy link
Member

ack 6663f7b

@aptrishu
Copy link
Member

@rultor merge

@rultor
Copy link

rultor commented Jan 13, 2017

@rultor merge

@aptrishu OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit 6663f7b into coala:master Jan 13, 2017
@rultor
Copy link

rultor commented Jan 13, 2017

@rultor merge

@aptrishu Done! FYI, the full log is here (took me 1min)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

8 participants