V2: WIP: Tests for Replacer

[aligator]

---

name: Tests for Replacer
about: Testing the Replacer

1. What does this change do, exactly?

This PR adds tests for the Replacer.

Currently there are tests for Set and Delete. (Both use ReplaceAll to check if they work, but I also want to write an extra test for it.)

Question: Should I test only through the Interface or the replacer type directly?

2. Please link to the relevant issues.

#2673
#2674

3. Which documentation changes (if any) need to be made because of this PR?

--

4. Checklist

• I have squashed any insignificant commits
• This change has comments explaining package types, values, functions, and non-obvious lines of code
• I am willing to help maintain this change if there are issues with it later

[CLAassistant]

All committers have signed the CLA.

[aligator]

Just noticed that I based the test on the old implementation. Updated it now to the new one (just used string(phOpen) instead of phOpen) and...
the tests fail:
replacer_test.go:81: Expectd 'val1123098765öö_äüspace valuetest-1231.2.3.4EMPTY{MyNotSetVariable}', got 'val1{asdf}098765{23456789}space value{1}1.2.3.4{testEmpty}{testEmpty}{MyNotSetVariable}' for input '{test1}{asdf}{äöü}{23456789}{with space}{1}{mySuper_IP}{testEmpty}{MyNotSetVariable}'

Seems that it always missed one, because my test string has no spaces between the } and {. This worked with your previous implementation.

Fix is in this branch https://github.com/aligator/caddy/tree/v2-fix-replacer but there seems to be at least one other bug because the second test fails even with the fix.

[mholt]

Nice start! I like that you're using table-driven tests.

Question: Should I test only through the Interface or the replacer type directly?

I would test the concrete replacer type, since in the tests we actually care about the implementation.

Here are a few ideas for things we need to test:

• NewReplacer() should make the static map and set two initial providers: https://sourcegraph.com/github.com/caddyserver/caddy@b44a22a9d45443aa736a04495dfe716b62b58adc/-/blob/replacer.go#L33

• Map() should add the given function to the providers list. https://sourcegraph.com/github.com/caddyserver/caddy@b44a22a9d45443aa736a04495dfe716b62b58adc/-/blob/replacer.go#L51

• Set() should add the key and value to the static map. https://sourcegraph.com/github.com/caddyserver/caddy@b44a22a9d45443aa736a04495dfe716b62b58adc/-/blob/replacer.go#L56

• Delete() should remove the key from the static map. https://sourcegraph.com/github.com/caddyserver/caddy@b44a22a9d45443aa736a04495dfe716b62b58adc/-/blob/replacer.go#L62

• ReplaceAll() should work on any and all inputs, meaning that valid placeholders should be replaced, respecting empty if set; and invalid/unrecognized placeholders should not be replaced. https://sourcegraph.com/github.com/caddyserver/caddy@b44a22a9d45443aa736a04495dfe716b62b58adc/-/blob/replacer.go#L75

In unit tests, don't call ReplaceAll when what you are testing is Set or Delete or anything other than ReplaceAll. Instead, to test Set, call Set() and then check the static map to ensure the value was set.

[aligator]

Ok, thank you for your suggestions. I will update my tests accordingly.

I just added one for something like {l{test1}...
This test fails with my current fixes in my fix-branch.

[mholt]

I just added one for something like {l{test1}...
This test fails with my current fixes in my fix-branch.

Excellent. :) That doesn't surprise me. You can either propose a fix in this PR or I can commit a fix. I'll be happy with either option.

[aligator]

@mholt could you please review this? I rewrote the tests and added new ones. Also I included two little fixes for the replacer logic.

[mholt]

Thanks for the updates! Looking much better.

[mholt]

Good thinking to test unicode

[mholt]

LGTM. Are you ready for this to be merged?

[aligator]

Yes, you can merge this.

[mholt]

Thanks for your contribution! It's nice to have that under some testing.