Page MenuHomePhabricator

Add Mocha TestEngine (not worth review / ready for upstream yet)
AbandonedPublic

Authored by tycho.tatitscheff on Aug 20 2015, 8:06 PM.
Tags
None
Referenced Files
F12832987: D13949.id.diff
Thu, Mar 28, 1:44 PM
F12831386: D13949.id.diff
Thu, Mar 28, 12:43 PM
Unknown Object (File)
Sat, Mar 16, 7:01 AM
Unknown Object (File)
Wed, Mar 6, 5:04 AM
Unknown Object (File)
Tue, Mar 5, 8:30 PM
Unknown Object (File)
Tue, Mar 5, 6:59 PM
Unknown Object (File)
Tue, Mar 5, 3:05 AM
Unknown Object (File)
Sun, Mar 3, 7:37 PM

Details

Reviewers
joshuaspence
epriestley
Group Reviewers
Blessed Reviewers
Summary
  • boostraping some mocha unit engine
  • fixing lint
Test Plan
  • write some mocha tests
  • run arc unit
  • go to party

Diff Detail

Repository
rARC Arcanist
Branch
feature/mochaTestEngine
Lint
Lint Errors
SeverityLocationCodeMessage
Errorsrc/unit/engine/MochaTestEngine.php:60XHP5Use of Undeclared Variable
Warningsrc/unit/engine/MochaTestEngine.php:60XHP9Naming Conventions
Advicesrc/unit/engine/MochaTestEngine.php:117XHP16TODO Comment
Advicesrc/unit/engine/MochaTestEngine.php:153XHP16TODO Comment
Unit
No Test Coverage
Build Status
Buildable 7700
Build 8437: [Placeholder Plan] Wait for 30 Seconds
Build 8436: arc lint + arc unit

Event Timeline

tycho.tatitscheff retitled this revision from to Add mocha parser.
tycho.tatitscheff updated this object.
tycho.tatitscheff edited the test plan for this revision. (Show Details)

Still WIP, and it is my first real contribution.
I hope I respected https://secure.phabricator.com/book/phabcontrib/article/contributing_code/
I know your guys are busy doing better things but if you find some times commenting the idea of this patch and the writing, please do.

tycho.tatitscheff retitled this revision from Add mocha parser to Add mocha test engine.Aug 20 2015, 10:16 PM

I hope I respected https://secure.phabricator.com/book/phabcontrib/article/contributing_code/

The first section of this document is:

The most important parts of contributing code to Phabricator are:

  • File a task with a bug report or feature request before you write code.

Can you help me understand why you didn't do that?

epriestley edited edge metadata.
This revision now requires changes to proceed.Aug 24 2015, 5:16 PM

I hope I respected https://secure.phabricator.com/book/phabcontrib/article/contributing_code/

The first section of this document is:

The most important parts of contributing code to Phabricator are:

  • File a task with a bug report or feature request before you write code.

Can you help me understand why you didn't do that?

I did speak of it in Z1336 and in irc and got no answer. So I diffed what I already did. Today I'm using it as extension in my project so don't worry, ever if it is trash for upstream it wont be lost time for me.

Basically I looked at the document later after diffing and take example of https://secure.phabricator.com/D12198 that haven't also a manifest task.

See irc log :

epriestley, i purpose D13949 without speaking to you first ? I just see this : "If you send us a patch without coordinating it with us first, it will probably be immediately rejected, or sit in limbo for a long time and eventually be rejected." Did this prevent me from beeing reviewed ?

tycho.tatitscheff retitled this revision from Add mocha test engine to Add Mocha TestEngine (not worth review / ready for upstream yet).Aug 25 2015, 6:20 PM
tycho.tatitscheff edited edge metadata.

Changed title so interessed users can allaways patch it.
Sorry for disturbing upstream with this.

joshuaspence edited edge metadata.

Some inlines. Most importantly this should wait for T5568, after which I would expect the API to stabilize a bit.

src/unit/engine/MochaTestEngine.php
2

There should be a blank line between <?php and any code, see D13534.

4

Prefer to write this as [[https://mochajs.org/ | Mocha]].

5

Prefer to write this as [[https://gotwarlost.github.io/istanbul/ | Istanbul]]

12

This isn't quite correct. @concrete-extensible only makes sense for non-final, non-abstract classes.

15–18

See my next comment.

23–25

Personally, I find this comment to be fairly useless and somewhat distracting. In this particular case, I don't think the comment provides any explanation that the code does not.

38–42

IMO, this doesn't provide any value.

48–50

As above.

54–56

As aabove.

59–62

As above.

65

Why not just if ($this->getEnableCoverage()) { ?

73–75

As above.

84

As above.

104

This should (later) come from arclint.

128–135

I'm not sure about &&... particularly I'm not sure if this will work on Windows.

138

This is equivalent to ExecFuture($cmd_line)

Ty @joshuaspence. I will fix this and commit it again. No problem for waiting, I did it wrong with no speaking with core first so if it end up landing upstring, it will be party for me !!

To be done :

  • add unit tests (shall I ?? it will submit mocha to this repo if i do so since I assume mocha is not installed globally)
  • fix nasty bugs (this currently bugs if there is no unit test or no line covered). I should allow having no unit tests or no covered lines, or at least prevent them from blocking arc unit
src/unit/engine/MochaTestEngine.php
38–42

Will get rid of all this useless comments :)

65

Indeed !

104

And get getConfigFromAnySource won't be froward compatible with .arclint and D13534 ?
I will look forward to see the correct API.

128–135

On production I have an other syntax that works on windows too. I will switch for this.

tycho.tatitscheff edited edge metadata.
  • Fixing issues raised on review

Fixing most of the requested changes ;)

isfs removed a subscriber: isfs.
isfs added a subscriber: isfs.