pact-foundation / pact-js

JS version of Pact. Pact is a contract testing framework for HTTP APIs and non-HTTP asynchronous messaging systems.
https://pact.io
Other
1.63k stars 348 forks source link

Improve storing logs for the test suite run #605

Open danilchican opened 3 years ago

danilchican commented 3 years ago

Checklist

Before making a feature request, I have:

Feature description

Currently pact.log is replaced every time pact test has ran.

Let's imagine that you already have pact tests and add one more. In case if one of the tests will fail, pact logs file can do not store logs for the failed test. It stores logs just for the latest test which was run.

I think it could be better if the logs file will contain all logs within a run test suite instead of replacing each time after specific test running.

Use case

In our project I faced with the described issue when I tried to add one more pact test and after running test suite I see that some tests are failed and I want to debug what happened. But when I opened logs file I saw that it stores only latest test run logs (not all of them). It's hard to debug. You need to rename or remove all pact tests except of failed and run test suite again but even in that case if you have more than one failing tests - in logs file will be logs for the only one.

mefellows commented 3 years ago

Most test frameworks have a "focus" or "skip" type feature (i.e. in mocha you can add the suffix .only) that lets you run the single test that failed. So it doesn't seem like a huge burden to me, but the idea of having a log file with all of the interactions for a test run could makes sense.

How would you like this to function? Should the log file ever be truncated or should it always be appended to, and the user should be responsible for deleting it?

danilchican commented 3 years ago

I think it would be good to have such flag in pact configuration like logsWriteMode with possible values overwrite, append/update and do not have possibility to store logs only for one test from test suite. It will be replaced with the following features like overwriting whole logs file or appending logs to existing one. Overwrite mode will clean up the logs file before writing.

TimothyJones commented 3 years ago

Jest pact helps this by giving each log file a different name, but you’re right-this could definitely be improved for the multi-suite use case.

I think we could work out a better-by-default method that would avoid the need to introduce additional config options

TimothyJones commented 3 years ago

If you’re not using jest, then a workaround could be to instantiate one pact instance per suite- with different log files. (This is what jest-pact does)

danilchican commented 3 years ago

@TimothyJones , yep. Even if default behavior will be improved to store all logs per suite like described above with overwrite option it's will be enough.

TimothyJones commented 3 years ago

@danilchican what test runner are you using?

danilchican commented 3 years ago

@TimothyJones, jest

TimothyJones commented 3 years ago

Oh, right. Please check out https://github.com/pact-foundation/jest-pact - it solves this problem for you.

danilchican commented 3 years ago

@TimothyJones , are you talking about -> https://github.com/pact-foundation/jest-pact#defaults ?

TimothyJones commented 3 years ago

Apologies, looks like it's not in the documentation. I will correct that. If you start each mockserver on a different port and don't specify a log file, then the log file is named accordingly.

danilchican commented 3 years ago

@TimothyJones , did you add this already in the documentation?

TimothyJones commented 3 years ago

https://github.com/pact-foundation/jest-pact

TimothyJones commented 3 years ago

Sorry, preemptive send. I can’t seem to do a direct link on mobile. It’s in the defaults section of: https://github.com/pact-foundation/jest-pact

danilchican commented 3 years ago

@TimothyJones In previous version of my pact test I had a separate file with pact config of consumer, provider, mock url, etc. But using this pact-jest package just for logs I have to use withPact method to wrap my consumer tests and put into method object with config. Is there any possibility to load config from separate file for withPact as it was done before through --setupFiles ./config/pact/setup.js? I don't want to pass and duplicate config properties every time in every consumer test.

TimothyJones commented 3 years ago

Ah, right. I see the problem.

The --setupFiles approach has a couple of drawbacks. It means the Pact mock is spun up for all specs, which adds time and noise to your tests. Additionally, it doesn't work especially well in cases where you need different settings for each spec (as you're currently experiencing). These are the problems that jest-pact was introduced to solve - but there are other workarounds (for example, splitting your specs into pact and non-pact). And, you raise a good point about not wanting to refactor your existing tests away from the --setupFiles method.

Using jest-pact with common config

The intention for multiple specs with common configuration in jest-pact is to have a common config object exported by a module:

import { withPact } from `jest-pact`;
import { pactConfig } from './__fixtures__/yourConfig';

withPact({...pactConfig, port: 8082 }, () => { /* etc */ });

However, long term we'd like to introduce / expose the config in a first-class way - see here for a proposal.

Solutions without migrating away from setupFiles

A naive way would be to add a random string to the log filename, but it would be annoying to tie those to the specs, and you would have to ensure you clear out the log directory before you run the tests.

I'll have a think about whether there's a way to write a --setupFiles module that would have sensible and repeatable log filenames for different tests. I don't think Jest exposes which spec is running. Hmm.

As Matt said above, you can also work around this by running just the spec you're interested in - although we realise that's not ideal.

Append log mode

As you said above, introducing an append log mode would also solve the problem in your case. Like the previous approach, a drawback would be that you'd have to ensure that the log files are cleaned out before running the tests. I'd really like the logs to work out of the box without extra config, though- making it configurable feels like we're leaking complexity to the user that would be better handled with sensible defaults (also- for technical reasons, adding this mode happens to not be a small change - but that doesn't mean we shouldn't do it, of course).

Obviating the need for the logs

As an aside- soon we'll have better exposition of the diff during the output of a failed test, which will mean needing to read the log file will happen less frequently.