727 into csv travis fix #843
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #843 +/- ##
===========================================
+ Coverage 67.71% 67.72% +0.01%
===========================================
Files 1 1
Lines 9878 9879 +1
Branches 2947 2948 +1
===========================================
+ Hits 6689 6691 +2
+ Misses 3189 3188 -1
Continue to review full report at Codecov.
|
|
It appears there are no longer CI errors! Not sure what changed. |
test/test612.js
Outdated
|
|
||
| it('With quote = \'\', single multiword string value', function(){ | ||
| alasql("SELECT 'swing out' AS `colname` INTO CSV('test612-1', {quote:''})"); | ||
| fs.open('test612-1.csv', 'r', function(err, fd) { |
There was a problem hiding this comment.
Looks really good with the tests.
A) Interacting with files is always async, so technically when fs starts reading the files we are not sure if the file has been written by the alasql command - so we need to use an async notation. replace alasql(...) with alasql.promise(...) and add .then(function(){ ... (code to assert things ... ) })
B) when testing async code the test must call first parameter passed from the it(...) function. We normally would change function(){ to function(done){ and then call done() as the last part after the assert
B) Next time you read a file you can avoid all the hassle by just using fs.readFileSync(file[, options])
The whole fs. thing can be changed to
assert('colname\n?swing out?' === fs.readFileSync('test612-1.csv','utf8));
(and then call done() after)
There was a problem hiding this comment.
Thank you for that. I've made the changes, but I'm having issues with the assertions. Here's what the first it() call looks like for me now:
it('With quote = \'\', single string value', function(){
return alasql.promise("SELECT 'swing' AS `colname` INTO CSV('test612-0', {quote:''})")
.then(function(state) {
console.log(fs.readFileSync('test612-0.csv', 'utf8') + '\n\n');
assert(fs.readFileSync('test612-0.csv', 'utf8') === 'colname\nswing\n');
}, function(err) {});
});It appears that the string readFileSync() is pulling is not not what I imagine it to be because the assertion fails. I've also tried the following strings and various other combinations I've forgotten:
'colname\nswing\n\ufeff'
'colname\r\nswing\r\n\ufeff'
'colname\nswing\n\n
I'm at a loss for how to write the assertion. Any ideas on where I might be going wrong with this?
Also, I chose the return the Promise rather than use done() because I read that mocha will catch the errors automatically. That part seems to be working now, whereas I was getting errors using done() as you advised.
There was a problem hiding this comment.
Dont worry, we will get there.
- As the alasql call is async we need to givethe testframework a way of knowing when the call is done. We do that by calling the fist parameter of the callback function to the
it(...function. Its not named at the moment. Lets call itdone
it('With quote = \'\', single string value', function(done){
```
and then call it after the assert with `done()`
- The `then(` function does not hvae a second parameter, but return a promise where we can `catch` errors.
- Also, reading a file is expensive. If you need the data twice
- The assert function can take a true/false (you get that by the `===`) but it works better if you give each side as a parameter - because then the test will output what was expected and what we got instead (making it easyer to debug)
In all I suggest the code looks like
```js
it('With quote = \'\', single string value', function(done ){
alasql.promise("SELECT 'swing' AS `colname` INTO CSV('test612-0', {quote:''})")
.then(function(state) {
var filecontent = fs.readFileSync('test612-0.csv', 'utf8');
assert(filecontent,'colname\nswing\n');
done();
}).catch(err) {
//console.error(err)
}
;
});
```
I dont get the `\ufeff` part of the code you are trying to assert
|
The Travis sometimes has a timeout error. I asked to rerun the test and it turned it all green :-) |
|
It seems to me that we need to clarify what the expected behavior is when For example, if With respect to the case where |
|
Hmm. Its a good question. I would say that for
I would expect a its the quotechar that must be escaped. Im not super sure about it, so please come with inputs. |
|
Hey Mathias, I've figured out how to get the tests and assertions to work correctly now. I've opted not to use the The And I suppose I see what you're saying regarding escaping the quote character within a field, whatever it happens to be. Right now, the code will always replace a quote character in the middle of the field with |
|
Good work! had no idea I could just return the promise...
guess you are right. Let me know when I can merge in - and we actually should make a test case for it too.. Ahh, haha, its the bom :) just give the CSV command the option |
|
Okay, fix applied to escape all quote characters other than |
|
Awwweeesommmme |
|
Once again, thank you so much for your patient assistance. You've helped me feel much more confident in contributing to an open source project. |
This PR should supersede #841 to fix #727 . The new tests do not cause errors on my local machine.
It seems that with commit af14329 only the Node.js 4 Travis build is failing for some reason.