Skip to content

[e2e]: Add external E2E tests#298

Merged
stp-ip merged 34 commits intotxtdirect:masterfrom
erbesharat:feature/e2e-tests
Dec 4, 2019
Merged

[e2e]: Add external E2E tests#298
stp-ip merged 34 commits intotxtdirect:masterfrom
erbesharat:feature/e2e-tests

Conversation

@erbesharat
Copy link
Copy Markdown
Member

@erbesharat erbesharat commented Oct 24, 2019 •
edited
Loading

What this PR does / why we need it:
Adds external e2e tests

Which issue this PR fixes:
fixes #291
fixes #203

Special notes for your reviewer:
Covered types:

  • host
  • path
  • gometa
  • dockerv2
  • git
  • proxy

Questions:

  • What should we do about adding the domains to /etc/hosts? Should we have a list of URIs in /e2e and write it to /etc/hosts when it gets changed? That way whenever we want to add a record to a test-case we have to add it to /e2e/URIs so the manager-script can read it and add them to /etc/hosts.

@erbesharat
Copy link
Copy Markdown
Member Author

erbesharat commented Oct 28, 2019

We should reconsider our flow for this test:

txtdirect/e2e/host/main.go

Lines 44 to 51 in 2164d93

{
name: "Redirect to a host record's unspecified to=",
args: data{
host: "noto.host.host.example.com",
path: "/",
},
expected: "http://noto.host.host.example.com/",
},

Right now since we don't return an error in getBaseTarget when to= is empty the test I mentioned gets redirected to itself.

Shouldn't we return an error in here so the fallback can get triggered?

txtdirect/txtdirect.go

Lines 53 to 62 in ec59e2c

func getBaseTarget(rec record, r *http.Request) (string, int, error) {
if strings.ContainsAny(rec.To, "{}") {
to, err := parsePlaceholders(rec.To, r, []string{})
if err != nil {
return "", 0, err
}
rec.To = to
}
return rec.To, rec.Code, nil
}

@erbesharat erbesharat mentioned this pull request Nov 6, 2019
Closed
@erbesharat
Copy link
Copy Markdown
Member Author

erbesharat commented Nov 6, 2019

Currently, the old fallback tests fail and there is a permission issue with Travis. Will migrate the fallback tests at the end after we have tests for all the types.

stp-ip
stp-ip reviewed Nov 9, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@stp-ip stp-ip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall great. Still missing a few tests as you mentioned, but feels much better separated and we have the ability to go much more e2e with this.

Valid choice to go with more code duplication, but better separation between tests.

What's your stance on newlines? I would generally say a newline for all files should be done. We are not doing it for dockerfiles, zonefiles, corefiles etc.

Awesome URL structure for the test redirects. Readable and understandable for both the request URL and the response one.

Will take a look into the Travis permission failures now.

e2e/gometa/zonefile
@ 86400 IN NS ns.example.net.
@ 86400 IN NS ns.example.nl.

$TTL 1H
Copy link
Copy Markdown
Member

@stp-ip stp-ip Nov 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$TTL 1H
$TTL 1h

Copy link
Copy Markdown
Member

@stp-ip stp-ip Dec 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still valid ;)

e2e/host/main.go Outdated
host: "noto.host.host.example.com",
path: "/",
},
expected: "http://noto.host.host.example.com/",
Copy link
Copy Markdown
Member

@stp-ip stp-ip Nov 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we expect this?

Copy link
Copy Markdown
Member Author

@erbesharat erbesharat Nov 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in my first comment, we should reconsider this test (it was in our old e2e tests) and return an error and trigger the fallback when to= is empty.

Copy link
Copy Markdown
Member

@stp-ip stp-ip Nov 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be 404 in general with no other fallback configured?

Copy link
Copy Markdown
Member

@stp-ip stp-ip Dec 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Copy Markdown
Member Author

@erbesharat erbesharat Dec 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

txtdirect/record.go

Line 192 in 97a881d

func ParseURI(uri string, w http.ResponseWriter, r *http.Request, c Config) string {

I thought this would take care of it but it only gets called when to= exist. Will add a condition to trigger fallback when the type is host and to= doesn't exist.

Copy link
Copy Markdown
Member

@stp-ip stp-ip Dec 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@stp-ip
Copy link
Copy Markdown
Member

stp-ip commented Nov 9, 2019

Additional notes:

github.com/docker/docker should be replaced with github.com/docker/engine within go.mod as far as I can see. Currently it works as we are using @master for the version instead of a tagged release, which would not be available within github.com/docker/docker.

make docker-build should use --network=host

Mounting the pkg directory as we do in our other docker run usage would speed up the tests.

ContainerCreate doesn't pull any container images. So it assumes that coredns/coredns for example is already present on the host. Either we do this within the makefile or the main.go file needs to pull in the specific container image before creating the container.

@ghost
Copy link
Copy Markdown

ghost commented Nov 16, 2019 •
edited by ghost
Loading

DeepCode Report (#ec80f2)

DeepCode analyzed this pull request.
There are no new issues.

stp-ip
stp-ip reviewed Nov 16, 2019
View reviewed changes
@erbesharat
Copy link
Copy Markdown
Member Author

erbesharat commented Nov 30, 2019

Sorry for the force pushes 😁 Didn't want to make more debugging commits.

@erbesharat erbesharat changed the title (WIP) [e2e]: Add external E2E tests [e2e]: Add external E2E tests Nov 30, 2019
@erbesharat erbesharat requested a review from stp-ip November 30, 2019 15:16
@erbesharat erbesharat mentioned this pull request Dec 3, 2019
Closed
stp-ip
stp-ip approved these changes Dec 4, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@stp-ip stp-ip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

merging as is to get this in and then we can iterate on dockerv2 as well as proxy.

@stp-ip stp-ip merged commit 6ddcdf4 into txtdirect:master Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stp-ip stp-ip stp-ip approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[tests]: Discuss external E2E tests [build] Create release version container images

2 participants

@erbesharat @stp-ip