-
Notifications
You must be signed in to change notification settings - Fork 80
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
feat: add pactffi_log_to_file for consumer/verifier #428
base: master
Are you sure you want to change the base?
Conversation
native/ffi.cc
Outdated
level = LevelFilter::LevelFilter_Trace; | ||
break; | ||
default: | ||
std::string err = "Unable to parse Level Filter: "; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be better if it included more about the source (eg, pact-js-core C integration, and "log level" instead of "Level")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed 👍🏾
src/ffi/index.ts
Outdated
@@ -10,20 +10,40 @@ export const PACT_FFI_VERSION = '0.3.19'; | |||
let ffi: typeof ffiLib; | |||
let ffiLogLevel: LogLevel; | |||
|
|||
const initialiseFfi = (logLevel: LogLevel): typeof ffi => { | |||
const initialiseFfi = (logLevel: LogLevel, logFile?: string): typeof ffi => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be better if it were more explicit - either you have a log file, or you don't. The current form means that it's easy to misread the intermediate functions that don't touch log file.
The guideline I would recommend is that optional parameters exist only at a boundary - and their presence (or not) is handled in the function where they are optional.
src/ffi/index.ts
Outdated
logger.debug(`Initalising native core at log level '${logLevel}'`); | ||
ffiLogLevel = logLevel; | ||
ffiLib.pactffiInitWithLogLevel(logLevel); | ||
if (logFile) { | ||
const convertLogLevelToFfiLoglevelFilter = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variables shouldn't be named with verbs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a constant map, it should:
- Be declared outside all functions
- Be all caps
I think you could call it FFI_LOG_LEVEL_FILTER
. But also, I don't think I understand why it's here. Why not just use FfiLogLevelFilter
directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But also, I don't think I understand why it's here
I don't know 😅 probably not through an active design decision, just mushy mushy, get me some loggy
src/ffi/index.ts
Outdated
); | ||
const res = ffiLib.pactffiLogToFile( | ||
logFile, | ||
convertLogLevelToFfiLoglevelFilter[logLevel] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if convertLogLevelToFfiLoglevelFilter[logLevel]
is undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this whole inlined convertLogLevelToFfiLoglevelFilter
constant would be better replaced with a switch
- then it would introduce no new concepts, and would give you error handling for free.
In general, if you need a really long name for something then it's a good signal that there's an alternative design that would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bad things probably 😂 or possibly, I don't know. Makes sense to test all the cases. I just did the bare minimum I could to get logging to file into pact-js when I was in the middle of diving into learning about FFI interfaces, so it is most certainly not the correct way, nor finished, I should move it to a draft and then it can be picked up either by me, or others if they feel inclined
test/consumer.integration.spec.ts
Outdated
@@ -188,7 +192,9 @@ describe('FFI integration test for the HTTP Consumer API', () => { | |||
interaction.withQuery('someParam', 0, 'someValue'); | |||
interaction.withRequestBinaryBody( | |||
bytes, | |||
isWin ? 'application/octet-stream' : 'application/gzip' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realise this was already this way when you got here, but there should not be if statements in tests - especially on platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally accept that and agreed, it points to a wider issue we have with content matching both cross platform and cross arch.
It's important to have these tests in, rather than skipping them, but raising these as discussion points so we can either fix the issue at source in the rust core, or provide platform/arch specific guidance where it is required.
This is a smell here, that is going to affect end users, unless they are aware of it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
failing making this change, there are just failing tests for something unrelated to the contributors change, which leads to a sub-optimal experience all round :(
Looks alright to me. I left a number of style comments inline.
Note that this ends up in the changelog, and this message describes the internals of the change, not how users of pact-js-core will see the change. I think it should describe how the users of pact-js-core will see the change.
See my comments inline. I don't think it's a good idea to pass optional parameters down to child functions. I also think that introducing an options object just for this seems unnecessary (and is kind of a workaround for the child function problem). Is there some reason you can't use the same pattern that the rest of the options uses? |
Thanks for the review dude will address these when I get time :) |
36a42e3
to
6ea56d5
Compare
note logging options are set globally and therefore cannot be updated between tests, this is a limitation of the current pact rust core implementation
6ea56d5
to
9acf466
Compare
Add ability to log to file, via pactffi_log_to_file
Will go someway to address pact-foundation/pact-js#954
(will need adding into pact-js)
Passed in via an optional param, to
getFfiLib
which is called by a couple of functions, as an optional param. Wonder if it would be better to have these in an opts object that can take named params.note, the option used to be called
log
in pact-js v2 and is moved to deprecated now. should we retainlog
or use a more descriptivelogFile
https://github.com/pact-foundaton/pact-js/tree/9.x.x#constructor
edit
need to add some appropriate tests and refactor based on review comments :)