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

Changed FileLoggerAdapterSpec to fail gracefully on Windows #946

Merged
merged 1 commit into from
Mar 10, 2016

Conversation

aneeshd16
Copy link
Contributor

Running tests on Windows caused this error:

B:\Projects\Parse Server\parse-server\spec\FileLoggerAdapter.spec.js:38
        expect(results[0].message).toEqual('testing info logs');
                         ^

TypeError: Cannot read property 'message' of undefined
    at B:\Projects\Parse Server\parse-server\spec\FileLoggerAdapter.spec.js:38:26
    at ParsePromise.<anonymous> (B:\Projects\Parse Server\parse-server\src\Adapters\Logger\FileLoggerAdapter.js:9:17440)
    at ParsePromise.wrappedResolvedCallback (B:\Projects\Parse Server\parse-server\node_modules\parse\lib\node\ParsePromise.js:139:41)
    at ParsePromise.resolve (B:\Projects\Parse Server\parse-server\node_modules\parse\lib\node\ParsePromise.js:72:36)
    at resolveOne (B:\Projects\Parse Server\parse-server\node_modules\parse\lib\node\ParsePromise.js:471:29)
    at ParsePromise.object.then.errors.(anonymous function) (B:\Projects\Parse Server\parse-server\node_modules\parse\lib\node\ParsePromise.js:480:13)
    at ParsePromise.wrappedResolvedCallback (B:\Projects\Parse Server\parse-server\node_modules\parse\lib\node\ParsePromise.js:139:41)
    at ParsePromise.resolve (B:\Projects\Parse Server\parse-server\node_modules\parse\lib\node\ParsePromise.js:72:36)
    at ReadFileContext.callback (B:\Projects\Parse Server\parse-server\src\Adapters\Logger\FileLoggerAdapter.js:9:16189)
    at FSReqWrap.readFileAfterOpen [as oncomplete] (fs.js:303:13)

Rest of the tests could not be run as the test runner would break here. This change adds a check to fail when the FileLoggerAdapter returns an empty array from here: https://github.com/ParsePlatform/parse-server/blob/master/src/Adapters/Logger/FileLoggerAdapter.js#L191

Regarding the cause of the error itself, it is due to different filename separators in *nix and Windows. The FileLoggerAdapter would not save logs (have not tested this). This is a separate issue and should also be fixed.

Running tests on Windows caused this error:
```
B:\Projects\Parse Server\parse-server\spec\FileLoggerAdapter.spec.js:38
        expect(results[0].message).toEqual('testing info logs');
                         ^

TypeError: Cannot read property 'message' of undefined
    at B:\Projects\Parse Server\parse-server\spec\FileLoggerAdapter.spec.js:38:26
    at ParsePromise.<anonymous> (B:\Projects\Parse Server\parse-server\src\Adapters\Logger\FileLoggerAdapter.js:9:17440)
    at ParsePromise.wrappedResolvedCallback (B:\Projects\Parse Server\parse-server\node_modules\parse\lib\node\ParsePromise.js:139:41)
    at ParsePromise.resolve (B:\Projects\Parse Server\parse-server\node_modules\parse\lib\node\ParsePromise.js:72:36)
    at resolveOne (B:\Projects\Parse Server\parse-server\node_modules\parse\lib\node\ParsePromise.js:471:29)
    at ParsePromise.object.then.errors.(anonymous function) (B:\Projects\Parse Server\parse-server\node_modules\parse\lib\node\ParsePromise.js:480:13)
    at ParsePromise.wrappedResolvedCallback (B:\Projects\Parse Server\parse-server\node_modules\parse\lib\node\ParsePromise.js:139:41)
    at ParsePromise.resolve (B:\Projects\Parse Server\parse-server\node_modules\parse\lib\node\ParsePromise.js:72:36)
    at ReadFileContext.callback (B:\Projects\Parse Server\parse-server\src\Adapters\Logger\FileLoggerAdapter.js:9:16189)
    at FSReqWrap.readFileAfterOpen [as oncomplete] (fs.js:303:13)
```
Rest of the tests could not be run as the test runner would break here. This change adds a check to fail when the FileLoggerAdapter returns an empty array from here: https://github.com/ParsePlatform/parse-server/blob/master/src/Adapters/Logger/FileLoggerAdapter.js#L191

Regarding the cause of the error itself, it is due to different filename separators in *nix and Windows. The FileLoggerAdapter would not save logs (have not tested this). This is a separate issue and should also be fixed.
@codecov-io
Copy link

Current coverage is 90.56%

Merging #946 into master will decrease coverage by -0.79% as of 3e5542e

@@            master    #946   diff @@
======================================
  Files           73      73       
  Stmts         4324    4324       
  Branches       863     863       
  Methods          0       0       
======================================
- Hit           3950    3916    -34
  Partial         10      10       
- Missed         364     398    +34

Review entire Coverage Diff as of 3e5542e

Powered by Codecov. Updated on successful CI builds.

@flovilmart
Copy link
Contributor

This is really bad to change the spec to accommodate the problem you're facing. please propose a PR that solved the problem instead of hiding it.

@flovilmart flovilmart closed this Mar 10, 2016
@aneeshd16
Copy link
Contributor Author

The problem i am facing is that the test runner breaks while running tests on Windows. This is solely concerned with how this spec is written. If there is something wrong in the code, the appropriate tests should fail. Bugs in the code should not make the entire test suite unusable.

please propose a PR that solved the problem instead of hiding it.

I agree that there exists a bug in the code that needs to be fixed, and I hope someone from the community comes up with a fix. This does not hide the bug, rather it highlights it by making this test fail. This change improves the spec by catching a failure it did not before. It makes the test fail where it should.

@flovilmart
Copy link
Contributor

You can propose the fix, to build paths that are functional in windows use

path.resolve and path.sep this should be a 1-2 liner, and I can merge it as soon as you propose it :)

I don't have a windows machine, so I can't really test that the fix would work but

https://github.com/ParsePlatform/parse-server/blob/master/src/Adapters/Logger/FileLoggerAdapter.js#L16

let LOGS_FOLDER = './logs/';

if (typeof process !== 'undefined' && process.env.NODE_ENV === 'test') {
  LOGS_FOLDER = './test_logs/'
}

You could replace by:

let path = require('path')
let BASE_LOGS_FOLDER = 'logs'
if (typeof process !== 'undefined' && process.env.NODE_ENV === 'test') {
  BASE_LOGS_FOLDER = 'test_logs'
}

let LOGS_FOLDER = path.join('.', path.sep, BASE_LOGS_FOLDER, path.sep);

@flovilmart flovilmart reopened this Mar 10, 2016
flovilmart added a commit that referenced this pull request Mar 10, 2016
Changed FileLoggerAdapterSpec to fail gracefully on Windows
@flovilmart flovilmart merged commit 2300b21 into parse-community:master Mar 10, 2016
@aneeshd16 aneeshd16 deleted the patch-2 branch March 10, 2016 17:21
@steven-supersolid
Copy link
Contributor

@flovilmart I'm trying your fix although it does not work. Will make a new PR if I can get it working - that is, the fix to the root problem

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

Successfully merging this pull request may close these issues.

5 participants