Added --monkeypatch option to pact mock service start up command and … #60

oliver-gramberg · 2017-11-10T11:44:05Z

…pact provider verifier command

These changes have been made blind, as I have not yet been able to set up an environment where I can compile and test (help appreciated), but they have been reviewed by our local TS expert :-)

…pact provider verifier command

oliver-gramberg · 2017-11-10T12:24:28Z

"when user specifies valid options
1) should return serverFactory using specified options"

fails with

"1) Pact Spec Create serverFactory when user specifies valid options should return serverFactory using specified options:
Error: Monkeypatch not found at path: /absolute/path/to/ruby/patch.rb
at Object.Server.create [as default] (src\server.ts:130:11)
at Pact.createServer (src\pact.ts:30:29)
at Context. (src\pact.spec.ts:94:23)"

because the named file obviously doesn't exist. Not sure how to specify a file such that it will be found in the builds.

oliver-gramberg · 2017-11-10T13:07:52Z

Now,

"when user specifies invalid log
1) should return an error on invalid path"

fails with

Pact Spec Create serverFactory when user specifies invalid log should return an error on invalid path:
AssertionError: expected [Function] to throw Error
at Context. (src\pact.spec.ts:168:16)"

The origin version of the test,

pact.createServer({log: path.resolve(dirPath, "log.txt")});
expect(fs.statSync(dirPath).isDirectory()).to.be.true;

seemed not to do what its spec said, so I tried to fix it with,

expect(() => {
	pact.createServer({log: path.resolve(dirPath, "log.txt")});
}).to.throw(Error);

but since it does

try {
	fs.statSync(path.normalize(options.dir)).isDirectory();
} catch (e) {
	mkdirp.sync(path.normalize(options.dir));
}

it never fails - unless I find a name that is invalid in all possible environments. Linux only forbids '/' in file/dir names, but any '/' I use will be swallowed by path.normalize().

I propose to delete this test. Will comment it out for now.

bethesque · 2017-11-14T20:30:37Z

Yes, I think the mock just creates whatever path you give it for the log.

mefellows · 2017-11-14T20:35:39Z

Thanks for this @oliver-gramberg, it mostly looks OK to me. There's another fundamental PR in the works at the moment, this is probably best done after that.

bethesque · 2017-11-14T20:58:18Z

Can you get the PWD via a function and prepend it to the relative path of the monkeypatch file to get its absolute path? I would actually expect there to be an absolute path function in the file utils class.

oliver-gramberg · 2017-11-16T15:05:33Z

@bethesque, sorry not sure where exactly you want me to do that. I did not find the place where the CLI is being called. Different repo?

bethesque · 2017-11-16T20:52:47Z

Not sure how to specify a file such that it will be found in the builds.

I was suggesting a solution for that. Something like "${Dir.pwd}/relative/path/to/file.rb".

Can I make a suggestion here? You seem to be testing the functionality, not the "contract" (interface). All your test needs to do is show that the absolute path is passed correctly to "--monkeypatch", rather than prove that the behaviour is correct. Along the lines of: https://docs.pact.io/best_practices/contract_tests_not_functional_tests.html

The tests that prove that the monkeypatch actually works are in the ruby codebase itself.

bethesque · 2017-11-16T20:58:56Z

I did not find the place where the CLI is being called.

I'm not actually sure how it works, as I don't maintain the js repos, sorry.

oliver-gramberg · 2017-11-17T08:25:07Z

OK, it's about the test code, not the production code. That answers my "where" question, at least partly. I was assuming the opposite, thinking you wanted me to actually get the monkeypatch parameter set.

Always happy to learn! I like the argument the article you referenced makes, and I think I will actually be able to apply it my current project. For this fork/PR: I copied existing test cases and adapted them to use the monkeypatch parameter. Yes, it's testing the functionality (of the wrapper code), but so are the tests where I copied from, and I am not testing deeper than those. Or am I? If I were to test the correct implementation of the monkeypatch, then shouldn't I have coded an expectation about the non-mangling of HTTP headers?

I am still not sure we're talking about the exact same pieces of code. Could we use the commenting feature of Github on the code diff tab of the PR?

bethesque · 2017-11-19T22:23:23Z

src/verifier.spec.ts

+				}).to.throw(Error);
+			});
+		});
+


The CLI can really only return a couple of responses. It can return an exit code 0, or an exit code 1. The only thing you can do if you get an error response is to spit out the error message, and fail the calling code. Personally, I wouldn't have this test above, because all the "how I handle the exit error code" tests are going to be the same. All you need to test is "how should the client behave when it gets an error", not "what are all the different things that can cause an error". Theoretically, the underlying Ruby process could change its implementation to just ignore a missing monkeypatch file, and then this test would fail, so it's not actually testing something pertinent to the js codebase.

This test exercises src/server.ts:125 ff. where I check if the named file exists. If not, the CLI is never called, but an Error is thrown.

bethesque · 2017-11-19T22:24:57Z

I've added a comment next to the bit of code that made me think of the "test the interface, not the behaviour" approach.

mefellows · 2018-02-12T19:42:18Z

Bump, what are we thinking on this issue folks?

bethesque · 2018-02-12T20:20:24Z

I would like it in please. We don't need to advertise the feature, but it helps us work around limitations of certain libraries (eg. Rack::Test which expects headers to always be in Dasherized-Capitalized-Format)

mboudreau · 2018-02-14T04:49:03Z

I'm really not a fan of fixing this level since it is something inherently wrong with the core library, hence any language specific implementation that uses it will break. Would be more efficient to fix it at the core.

bethesque · 2018-02-15T02:48:15Z

it is something inherently wrong with the core library - there is an assumption in Rack::Test that all HTTP headers follow the same format. While it is recommended and common to use capitalized words with dashes, it is not actually mandatory, and there is at least one person out there who can't use Pact because of this code, as they use lower case and underscores. I am not in a position to change the behaviour of Rack::Test, as it is the most widely used HTTP testing library in Ruby, and for 99% of use cases, they are correct to make the assumption that they do. I can, however, provide a way for the probably less than 1% of users to alter the code to allow them to use our library.

mboudreau · 2018-02-15T02:56:23Z

Sure, I understand that, but again, all libraries that are using the core will have this issue, so instead of fixing it only in pact-node, how about we fix it closer to the root cause?

…

On Thu, Feb 15, 2018 at 1:48 PM Beth Skurrie ***@***.***> wrote: it is something inherently wrong with the core library - there is an assumption in Rack::Test that all HTTP headers follow the same format. While it is recommended and common to use capitalized words with dashes, it is not actually mandatory, and there is at least one person out there who can't use Pact because of this code, as they use lower case and underscores. I am not in a position to change the behaviour of Rack::Test, as it is the most widely used HTTP testing library in Ruby, and for 99% of use cases, they are correct to make the assumption that they do. I can, however, provide a way for the probably less than 1% of users to alter the code to allow them to use our library. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#60 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjA5LlBycyzThtCWA6QPujsMMPuX39Eks5tU5rvgaJpZM4QZcXg> .

bethesque · 2018-02-15T03:03:19Z

Are you suggesting that I fix the Ruby code in the standalone? As I've said, I can't fix it, and even if I could, changing the behaviour of that particular bit of code is not what we want to do for all users.

I can only provide a way to load a Ruby file that can alter the behaviour for their particular use case. To do this, I have added a --monkeypatch file argument to the Ruby standalone CLI. This PR is just exposes that option to the javascript code.

mefellows · 2018-02-18T22:41:39Z

@bethesque just thinking out loud here, could this be resolved by overriding the headers with the the --custom-provider-header flag?

Down the track, supporting "request filters" as a first class citizen could also be a nice catch here.

bethesque · 2018-02-18T22:58:21Z

I spent quite a lot of time looking in to this, and this was the only solution I could come up with.

bethesque · 2018-02-22T03:10:05Z

Here's an example of why this feature is really useful. I've been trying to fix this issue. The code that needs to be altered is embedded deep in the Ruby chain. By using the monkeypatch file, I can modify the code to allow me to test potential fixes without having to repackage everything (ruby, travelling ruby, js...) every time. Fast feedback loops lead to fast bug fixes. By exposing the monkeypatch option in the js, I'll be able to experiment with fixes really easily, or allow other people to try out fixes on their local machines if it's something that I can't reproduce (which seems to happen a lot with windows!)

mboudreau · 2018-02-25T22:16:46Z

Alright, fine, I'll get this done. Should we document this feature with an extreme warning or not document it at all so people don't abuse it?

…

On Thu, Feb 22, 2018 at 2:10 PM Beth Skurrie ***@***.***> wrote: Here's an example of why this feature is really useful. I've been trying to fix this <pact-foundation/pact-js#150> issue. The code that needs to be altered is embedded deep in the Ruby chain. By using the monkeypatch <pact-foundation/pact-ruby-standalone-windows-test@0352de0> file, I can modify the code to allow me to test potential fixes without having to repackage everything (ruby, travelling ruby, js...) every time. Fast feedback loops lead to fast bug fixes. By exposing the monkeypatch option in the js, I'll be able to experiment with fixes really easily, or allow other people to try out fixes on their local machines if it's something that I can't reproduce (which seems to happen a lot with windows!) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#60 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjA5Mwo3RJKn1bKUGO6XntJY8EX4WcMks5tXNqNgaJpZM4QZcXg> .

bethesque · 2018-02-25T22:31:58Z

Let's not document it. It'll only need to be used under "adult supervision" (ie. instructions from me!)

mboudreau · 2018-02-25T23:01:02Z

cool :)

…

On Mon, Feb 26, 2018 at 9:31 AM Beth Skurrie ***@***.***> wrote: Let's not document it. It'll only need to be used under "adult supervision" (ie. instructions from me!) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#60 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjA5KTQ-tyo5nWulwEDbv_Si_qOiTqCks5tYd9egaJpZM4QZcXg> .

mboudreau · 2018-02-28T06:11:55Z

I'm going to close this PR as I merged it manually because of all the major changes that had happened since and added more tests to make sure that the monkeypatch option actually works (thanks @bethesque for the mini test with the ruby file).

8701b4d

Version 6.11.0 will be going out soon with this.

Added --monkeypatch option to pact mock service start up command and …

16bf36f

…pact provider verifier command

Fix to broken test by providing an existing file name using __filename

cda0f6f

Commenting out unnecessary test for invalid log file path

5d2ecb2

oliver-gramberg mentioned this pull request Nov 14, 2017

HTTP header names mangled even if conformant to RFC pact-foundation/pact-mock_service#78

Closed

bethesque reviewed Nov 19, 2017

View reviewed changes

Oliver Gramberg added 4 commits November 20, 2017 14:57

Minute changes

68ccbf6

Merge branch 'master' into monkeypatch-mock-service

dc31cab

Minute changes

cb43da2

Minute change to rerun checks on Github (timed out on last push)

8347afa

Merge branch 'master' into monkeypatch-mock-service

24c16dd

mboudreau closed this Feb 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added --monkeypatch option to pact mock service start up command and … #60

Added --monkeypatch option to pact mock service start up command and … #60

oliver-gramberg commented Nov 10, 2017

oliver-gramberg commented Nov 10, 2017

oliver-gramberg commented Nov 10, 2017

bethesque commented Nov 14, 2017

mefellows commented Nov 14, 2017

bethesque commented Nov 14, 2017

oliver-gramberg commented Nov 16, 2017

bethesque commented Nov 16, 2017 •

edited

Loading

bethesque commented Nov 16, 2017

oliver-gramberg commented Nov 17, 2017 •

edited

Loading

bethesque Nov 19, 2017 •

edited

Loading

oliver-gramberg Nov 21, 2017

bethesque commented Nov 19, 2017

mefellows commented Feb 12, 2018

bethesque commented Feb 12, 2018

mboudreau commented Feb 14, 2018

bethesque commented Feb 15, 2018

mboudreau commented Feb 15, 2018 via email

bethesque commented Feb 15, 2018 •

edited

Loading

mefellows commented Feb 18, 2018

bethesque commented Feb 18, 2018

bethesque commented Feb 22, 2018

mboudreau commented Feb 25, 2018 via email

bethesque commented Feb 25, 2018

mboudreau commented Feb 25, 2018 via email

mboudreau commented Feb 28, 2018

Added --monkeypatch option to pact mock service start up command and … #60

Added --monkeypatch option to pact mock service start up command and … #60

Conversation

oliver-gramberg commented Nov 10, 2017

oliver-gramberg commented Nov 10, 2017

oliver-gramberg commented Nov 10, 2017

bethesque commented Nov 14, 2017

mefellows commented Nov 14, 2017

bethesque commented Nov 14, 2017

oliver-gramberg commented Nov 16, 2017

bethesque commented Nov 16, 2017 • edited Loading

bethesque commented Nov 16, 2017

oliver-gramberg commented Nov 17, 2017 • edited Loading

bethesque Nov 19, 2017 • edited Loading

Choose a reason for hiding this comment

oliver-gramberg Nov 21, 2017

Choose a reason for hiding this comment

bethesque commented Nov 19, 2017

mefellows commented Feb 12, 2018

bethesque commented Feb 12, 2018

mboudreau commented Feb 14, 2018

bethesque commented Feb 15, 2018

mboudreau commented Feb 15, 2018 via email

bethesque commented Feb 15, 2018 • edited Loading

mefellows commented Feb 18, 2018

bethesque commented Feb 18, 2018

bethesque commented Feb 22, 2018

mboudreau commented Feb 25, 2018 via email

bethesque commented Feb 25, 2018

mboudreau commented Feb 25, 2018 via email

mboudreau commented Feb 28, 2018

bethesque commented Nov 16, 2017 •

edited

Loading

oliver-gramberg commented Nov 17, 2017 •

edited

Loading

bethesque Nov 19, 2017 •

edited

Loading

bethesque commented Feb 15, 2018 •

edited

Loading