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

intermittent failing logger test on circleci #433

Closed
tripodsan opened this issue Jan 7, 2019 · 5 comments · Fixed by #469
Closed

intermittent failing logger test on circleci #433

tripodsan opened this issue Jan 7, 2019 · 5 comments · Fixed by #469
Labels
bug Something isn't working

Comments

@tripodsan
Copy link
Contributor

Description
happens only on circle ci...sometimes:

  1) Testing standard logger configuration
       File logger can be created:
     Uncaught Error: ENOENT: no such file or directory, open '/home/circleci/repo/test/tmp/testlog.log'
      at Object.fs.openSync (fs.js:646:18)
      at Object.fs.readFileSync (fs.js:551:33)
      at Timeout.setTimeout [as _onTimeout] (test/testLogCommon.js:57:22)

Expected behavior
no errors.

@tripodsan tripodsan added the bug Something isn't working label Jan 7, 2019
@DrewML
Copy link

DrewML commented Jan 14, 2019

Likely related: winstonjs/winston#1504. Winston 2/3 historically has a ton of problems with answering the question "have all logs been flushed?"

Relevant: https://github.com/winstonjs/winston#awaiting-logs-to-be-written-in-winston (doesn't actually work with the File transport).

It seems like there is already test coverage for Winston file writes in helix-shared, though. Seems like testing the actual writes in helix-cli might be unnecessary?

@tripodsan
Copy link
Contributor Author

well, there are other cases, where we want to test the log output. so maybe a string based transport for winston would help here.

@tripodsan tripodsan assigned tripodsan and unassigned tripodsan Jan 15, 2019
@DrewML
Copy link

DrewML commented Jan 16, 2019

maybe a string based transport for winston would help here.

Seems like that would make more sense imho. I assume you'd want an upstream change in @adobe/helix-shared to add the option for some form of string based transport? It seems like the usage of winston is just an implementation detail of helix-shared so I wouldn't want to reach in and muck with transports.

@tripodsan
Copy link
Contributor Author

I added a string based test logger here: ce67700

I would disable the common log tests here and wait until winstonjs/winston#1504 is fixed.
another issue might arise, if the winston versions used in shared and cli are not the same. this happens to me a lot when npm linking the helix-shared during development. then the winston.loggers.clear() doesn't work and the tests fail.

It seems like the usage of winston is just an implementation detail of helix-shared so I wouldn't want to reach in and muck with transports.

exactly. I suggest that we remove the winston dependency from helix-cli and only rely on helix-shared. so maybe create a string transport when passing /dev/text or something :-)

@tripodsan
Copy link
Contributor Author

I created adobe/helix-shared#33

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants