Expose versions data through the collection API #1033
Conversation
Ndpnt
force-pushed
the
versions-api
branch
4 times, most recently
from
1648eb5 to
b65aeae
Compare
|
|
||
| app.listen(port); | ||
|
|
||
| if (process.env.NODE_ENV !== 'test') { |
There was a problem hiding this comment.
Why do we refuse to start the server when testing? 🤔 This is counter-intuitive and might lead to a lot of wasted time.
At least, a comment should be added explaining the rationale and a message should be output to let the user know that the server is not starting in test mode.
There was a problem hiding this comment.
Event when testing the server is started, there is only no server log to avoid polluting the test log.
There was a problem hiding this comment.
Thank you, I clearly misread the line below. Sorry for the noise.
I believe this would be better addressed by changing the logger config in test mode rather than changing the instructions: always ask to log, but the logger decides what is output in test mode (perhaps nothing).
There was a problem hiding this comment.
Thanks! The test classes seem relevant to me 🙂
I personally would support a slightly more terse version of those tests where we would test in a single
itblock that the status code, the content-type and the content are as the expected ones. The current approach is certainly more precise and enables exact pinpointing of potential issues. However, those tests are also expensive to create and maintain (see for example here the mistake in some test descriptions). I would not see a problem decreasing the quality by grouping those tests and having less description text to maintain 🙂This is not a request for changes.
There was a problem hiding this comment.
Reviewed and tested locally, works well! 👏
I did not check the upgrade instructions.
I found one interesting inconsistency that, from my point of view, does not need fixing but is worth noting: the data sources for /services and for /version are not the same. One is the declarations, the other is the versions. This means that, if the config is inconsistent (loading declarations and versions from two different collections), the return values will be inconsistent, with a service+type that was exposed in /services being unavailable under `/version.
|
|
||
| app.listen(port); | ||
|
|
||
| if (process.env.NODE_ENV !== 'test') { |
There was a problem hiding this comment.
Thank you, I clearly misread the line below. Sorry for the noise.
I believe this would be better addressed by changing the logger config in test mode rather than changing the instructions: always ask to log, but the logger decides what is output in test mode (perhaps nothing).
Co-authored-by: Matti Schneider <matti@opentermsarchive.org>
Co-authored-by: Matti Schneider <matti@opentermsarchive.org>
Implements #1028.