feat: add pactffi_log_to_file for consumer/verifier #428

YOU54F · 2023-01-09T14:01:27Z

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 retain log or use a more descriptive logFile

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 :)

TimothyJones · 2023-01-09T14:57:08Z

native/ffi.cc

+      level = LevelFilter::LevelFilter_Trace;
+      break;
+    default:
+      std::string err =  "Unable to parse Level Filter: ";


This would be better if it included more about the source (eg, pact-js-core C integration, and "log level" instead of "Level")

agreed 👍🏾

TimothyJones · 2023-01-09T15:01:45Z

src/ffi/index.ts

@@ -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 => {


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.

TimothyJones · 2023-01-09T15:02:43Z

src/ffi/index.ts

  logger.debug(`Initalising native core at log level '${logLevel}'`);
  ffiLogLevel = logLevel;
-  ffiLib.pactffiInitWithLogLevel(logLevel);
+  if (logFile) {
+    const convertLogLevelToFfiLoglevelFilter = {


Variables shouldn't be named with verbs.

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?

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

TimothyJones · 2023-01-09T15:06:24Z

src/ffi/index.ts

+    );
+    const res = ffiLib.pactffiLogToFile(
+      logFile,
+      convertLogLevelToFfiLoglevelFilter[logLevel]


What happens if convertLogLevelToFfiLoglevelFilter[logLevel] is undefined?

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.

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

TimothyJones · 2023-01-09T15:10:39Z

test/consumer.integration.spec.ts

@@ -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'


I realise this was already this way when you got here, but there should not be if statements in tests - especially on platform.

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

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 :(

TimothyJones · 2023-01-09T15:15:16Z

Looks alright to me. I left a number of style comments inline.

feat: add pactffi_log_to_file for consumer/verifier

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.

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.

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?

YOU54F · 2023-05-02T20:17:21Z

Thanks for the review dude will address these when I get time :)

note logging options are set globally and therefore cannot be updated between tests, this is a limitation of the current pact rust core implementation

TimothyJones reviewed Jan 9, 2023

View reviewed changes

YOU54F marked this pull request as draft May 2, 2023 20:21

YOU54F mentioned this pull request Sep 27, 2023

[Feature Enhancement] - v10.x - Support logging to file pact-foundation/pact-php#341

Open

YOU54F force-pushed the add_ffi_log_to_file branch from 36a42e3 to 6ea56d5 Compare June 25, 2024 21:51

feat: support logFile option to makeConsumerPact & verify functions

9acf466

note logging options are set globally and therefore cannot be updated between tests, this is a limitation of the current pact rust core implementation

YOU54F force-pushed the add_ffi_log_to_file branch from 6ea56d5 to 9acf466 Compare June 25, 2024 21:51

mefellows mentioned this pull request Jul 4, 2024

fix: reduce log level for matching to debug pact-foundation/pact-reference#454

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add pactffi_log_to_file for consumer/verifier #428

feat: add pactffi_log_to_file for consumer/verifier #428

YOU54F commented Jan 9, 2023 •

edited

Loading

TimothyJones Jan 9, 2023

YOU54F May 2, 2023

TimothyJones Jan 9, 2023 •

edited

Loading

TimothyJones Jan 9, 2023

TimothyJones Jan 9, 2023

YOU54F May 2, 2023 •

edited

Loading

TimothyJones Jan 9, 2023

TimothyJones Jan 9, 2023 •

edited

Loading

YOU54F May 2, 2023

TimothyJones Jan 9, 2023

YOU54F May 2, 2023

YOU54F May 2, 2023

TimothyJones commented Jan 9, 2023

YOU54F commented May 2, 2023

feat: add pactffi_log_to_file for consumer/verifier #428

Are you sure you want to change the base?

feat: add pactffi_log_to_file for consumer/verifier #428

Conversation

YOU54F commented Jan 9, 2023 • edited Loading

Add ability to log to file, via pactffi_log_to_file

edit

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyJones Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YOU54F May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyJones Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyJones commented Jan 9, 2023

YOU54F commented May 2, 2023

YOU54F commented Jan 9, 2023 •

edited

Loading

TimothyJones Jan 9, 2023 •

edited

Loading

YOU54F May 2, 2023 •

edited

Loading

TimothyJones Jan 9, 2023 •

edited

Loading