mozilla-comm / jsmime

A MIME parser written in JavaScript
MIT License
42 stars 18 forks source link

Use gulpfile to building things & introduce the text-encoding for testing under nodejs. #17

Open lygstate opened 8 years ago

lygstate commented 8 years ago

Now we could be able to introduce UTF7 and other needed encodings when necessary, Cause all the encodings are implement in pure js, so we could be able to running jsmime in the worker.

jcranmer commented 8 years ago

Unfortunately, I don't think Gulp is an acceptable build system here. All of the other libraries I've been playing with are using Grunt, not gulp, so I would not feel comfortable with a gulp build system.

jcranmer commented 8 years ago

Also, for future reference, your commits are way too coarse-grained. One commit per issue.

lygstate commented 8 years ago

For getting the pull request works, there must be such a big change, except the stringview.js, Or we have no unit-test make sure the thing works

lygstate commented 8 years ago

@jcranmer Anyway, please at least clone my fork & running the test & have a look at the effect.

jcranmer commented 8 years ago

As I mentioned earlier:

I really would rather be using Grunt instead of Gulp. And the main reason for this is that the other libraries that we are using or wish to use in comm-central (e.g., ical.js) are using Grunt, and so it makes sense to standardize on that (as well as Mocha tdd based testing), so that it would ease future integration into a potential new more JS-y comm-central build system.

lygstate commented 8 years ago

@jcranmer Please also consider the gulpfile, indeed, that's helps me a lost, and gulpfile just a building system, doesn't affect what's we would to do. I want to do much contribution on jsmime, and I am familar with gulpfile, and now it's works

lygstate commented 8 years ago

I've update the pull request, maybe easier to review.

jcranmer commented 8 years ago

You've still not addressed all of the concerns.

Saying that gulp "is just a building system" means you're missing the point of what I'm saying. You're basically trying to change the configuration of a system to suit your needs without considering the needs of other, extant uses of the library. I can already tell that even the new patch will still break the import into Thunderbird.

JSMime should already work with require.js and AMD modules (indeed, that's what the test you were trying to test was in fact doing). This makes me highly wary of your changes to support a library whose main purposes is to make Node modules work in the browser.

As for development, I've done all of my testing using a localhost server with the moch/chai tests in use by Firefox. E.g., the URL http://localhost/source/jsmime/test/browser/amd lets me run all of the tests (and find lots of bugs in the test driver, since my IDNA tests in another WIP patch uses lots of weird Unicode strings). And the only reason it requires a test server is because the CORS rules on file:// suck :-(

In short, you're basically completely changing the API of the project without trying to ensure that the current users of the API are still compatible.

jcranmer commented 8 years ago

FWIW (since I know you can build Thunderbird), the way I test that code works in Thunderbird is:

DEST=/src/trunk/comm-central/mailnews/mime/jsmime
node build/build.js
# Clean out the old import of jsmime
rm -rf $DEST/*
# Copy files to the destination
cp LICENSE README.md $DEST
cp dist/jsmime.js $DEST
find test -type f ! -path 'test/browser/*' | xargs tar c | tar x -C $DEST

If running xpcshell-tests causes a test fail, then the change cannot be accepted as it would break Thunderbird. This requirement is non-negotiable.

lygstate commented 8 years ago

@jcranmer So if that not breaks thunderbird? The patch can be accepted? From my design, it's woun't break the thunderbird, cause I am still output a UMD version of jsmime.js

lygstate commented 8 years ago

The header is something like this: (function(f){if(typeof exports==="object"&&typeof module!=="undefined"){module.exports=f()}else if(typeof define==="function"&&define.amd){define([],f)}else{var g;if(typeof window!=="undefined"){g=window}else if(typeof global!=="undefined"){g=global}else if(typeof self!=="undefined"){g=self}else{g=this}g.jsmime = f()}})(function(){var define,module,exports; I should re-check it.

lygstate commented 8 years ago

@jcranmer OK, I figure out a way to keep the old tests still running. waiting for tune.

lygstate commented 8 years ago

@jcranmer I've updated the pull request, now both nodejs test cases & firefox testcases are running. I preserve the original test cases, and change nothing except the array comprehension, cause babel doesn't support that.

lygstate commented 8 years ago

The firefox browser passed all tests, including AMD/Globals. Chrome can not running those tests.

The following result is for nodejs, only GB18030 decoding is incorrect, there is two case failure, but in other way, nodejs passed the BIG5 test case.

Along the nodejs testcase, the code coverage result are coming out. Running gulp coverage is enough.

   764 passing (3s)
  2 failing

  1) headerparser decodeRFC2047Words =?GB18030?B?lDnaMw==?=:

      AssertionError: '�' deepEqual '�'
      + expected - actual

      -�
      +�

      at Context.<anonymous> (test_header.js:632:16)
      at callFn (C:\CI-Cor\ks\jsmime\node_modules\gulp-mocha\node_modules\mocha\lib\runnable.js:286:21)
      at Test.Runnable.run (C:\CI-Cor\ks\jsmime\node_modules\gulp-mocha\node_modules\mocha\lib\runnable.js:279:7)
      at Runner.runTest (C:\CI-Cor\ks\jsmime\node_modules\gulp-mocha\node_modules\mocha\lib\runner.js:421:10)
      at C:\CI-Cor\ks\jsmime\node_modules\gulp-mocha\node_modules\mocha\lib\runner.js:528:12
      at next (C:\CI-Cor\ks\jsmime\node_modules\gulp-mocha\node_modules\mocha\lib\runner.js:341:14)
      at C:\CI-Cor\ks\jsmime\node_modules\gulp-mocha\node_modules\mocha\lib\runner.js:351:7
      at next (C:\CI-Cor\ks\jsmime\node_modules\gulp-mocha\node_modules\mocha\lib\runner.js:283:14)
      at Immediate._onImmediate (C:\CI-Cor\ks\jsmime\node_modules\gulp-mocha\node_modules\mocha\lib\runner.js:319:5)

  2) MimeParser Charset tests Charset conversion:

      AssertionError: 'ハローⷑワールド' == 'ハロー・ワールド'
      + expected - actual

      -ハローⷑワールド
      +ハロー・ワールド

      at test_mime_tree.js:613:18

Error in plugin 'gulp-mocha'
Message:
    2 tests failed.
-----------------------|----------|----------|----------|----------|----------------|
File                   |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
-----------------------|----------|----------|----------|----------|----------------|
 jsmime\               |    96.93 |    94.81 |    93.81 |    97.23 |                |
  encodings.js         |    66.67 |       50 |      100 |    66.67 |              8 |
  headeremitter.js     |    98.19 |    95.45 |      100 |    98.15 |  27,29,415,511 |
  headerparser.js      |    98.71 |    96.91 |      100 |    98.69 |... 78,710,1101 |
  jsmimeMain.js        |      100 |      100 |      100 |      100 |                |
  mimeparser.js        |    93.29 |    92.05 |    86.11 |    94.35 |... 821,898,899 |
  mimeutils.js         |      100 |      100 |      100 |      100 |                |
  structuredHeaders.js |    98.91 |      100 |    93.33 |    98.75 |            109 |
-----------------------|----------|----------|----------|----------|----------------|
All files              |    96.93 |    94.81 |    93.81 |    97.23 |                |
-----------------------|----------|----------|----------|----------|----------------|

=============================== Coverage summary ===============================
Statements   : 96.93% ( 1012/1044 )
Branches     : 94.81% ( 530/559 )
Functions    : 93.81% ( 91/97 )
Lines        : 97.23% ( 982/1010 )
================================================================================
[03:21:43] Finished 'coverage' after 5.06 s

C:\CI-Cor\ks\jsmime>
lygstate commented 8 years ago

@jcranmer any time?