Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: rewrite e2e test(test-e2e-route-with-method) using ginkgo #1675

Merged

Conversation

bisakhmondal
Copy link
Member

Please answer these questions before submitting a pull request, or your PR will get closed.

Why submit this pull request?

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches

Related issues
#1500

@codecov-io
Copy link

codecov-io commented Mar 26, 2021

Codecov Report

Merging #1675 (a30f011) into master (88f3232) will increase coverage by 1.55%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1675      +/-   ##
==========================================
+ Coverage   72.41%   73.96%   +1.55%     
==========================================
  Files         133       86      -47     
  Lines        5728     2612    -3116     
  Branches      666      666              
==========================================
- Hits         4148     1932    -2216     
+ Misses       1337      680     -657     
+ Partials      243        0     -243     
Flag Coverage Δ
backend-e2e-test ?
backend-e2e-test-ginkgo ?
backend-unit-test ?
frontend-e2e-test 73.96% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
api/internal/utils/runtime/runtime.go
api/internal/handler/global_rule/global_rule.go
api/internal/core/store/store.go
api/internal/handler/ssl/ssl.go
api/internal/utils/closer.go
api/internal/core/entity/format.go
api/internal/handler/tool/tool.go
api/internal/conf/conf.go
api/internal/filter/request_id.go
...pi/internal/handler/plugin_config/plugin_config.go
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88f3232...a30f011. Read the comment docs.

Copy link
Member

@nic-chen nic-chen left a comment

Choose a reason for hiding this comment

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

LGTM

Object: ManagerApiExpect(t),
Sleep: base.SleepTime,
}),
table.Entry("delete route", base.HttpTestCase{
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems the update of these routes is missing, we should add it.

Desc: "delete route",
Object: ManagerApiExpect(t),
Sleep: base.SleepTime,
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we may update the methods list and verify the change.

Copy link
Member Author

Choose a reason for hiding this comment

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

looking into it.

@bisakhmondal bisakhmondal requested a review from tokers March 26, 2021 16:07
@bisakhmondal
Copy link
Member Author

Hello guys,

Can't we edit the URI of a route after it has been created? We can, right?

It seems when we are editing/adding the existing/new URI nginx putting a 404 error, not the apisix.

it is how apisix put error for forbidden methods or non-existing URI
(before updating the URI)

image

But here (after editing)

image

Am I missing something here?
Thanks

@Jaycean
Copy link
Member

Jaycean commented Mar 26, 2021

Hello guys,

Can't we edit the URI of a route after it has been created? We can, right?

It seems when we are editing/adding the existing/new URI nginx putting a 404 error, not the apisix.

it is how apisix put error for forbidden methods or non-existing URI
(before updating the URI)

image

But here (after editing)

image

Am I missing something here?
Thanks

I have just tested in my local area, and there is no 404 error, so I think you can create a new issue and write out the created data and process. Let's discuss this problem

@bisakhmondal
Copy link
Member Author

Okay, @Jaycean, I'm opening an issue.
See the ci is also failing with a 404 for that particular test.
image

@tokers
Copy link
Contributor

tokers commented Mar 27, 2021

@bisakhmondal Please fix the go lint errors :).

@bisakhmondal
Copy link
Member Author

hi @tokers it's due to exceeding timeout, lol. Going to make a blank commit to retrigger the CIs

@tokers
Copy link
Contributor

tokers commented Mar 27, 2021

hi @tokers it's due to exceeding timeout, lol. Going to make a blank commit to retrigger the CIs

I see, I'm triggering the CI actions.

@gxthrj
Copy link
Contributor

gxthrj commented Mar 27, 2021

E2E test is failed.

@Jaycean
Copy link
Member

Jaycean commented Mar 27, 2021

Okay, @Jaycean, I'm opening an issue.
See the ci is also failing with a 404 for that particular test.
image

I see this problem. Let me find out the reason.

"upstream": {
"type": "roundrobin",
"nodes": [{
"host": "172.16.238.20",
Copy link
Member

@Jaycean Jaycean Mar 27, 2021

Choose a reason for hiding this comment

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

nodes host please use the base.UpstreamIp

"nodes": {
			"` + base.UpstreamIp + `:1980": 1
		}

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch 👍. Update pushed.

api/test/e2enew/route/route_with_methods_test.go Outdated Show resolved Hide resolved
api/test/e2enew/route/route_with_methods_test.go Outdated Show resolved Hide resolved
api/test/e2enew/route/route_with_methods_test.go Outdated Show resolved Hide resolved
api/test/e2enew/route/route_with_methods_test.go Outdated Show resolved Hide resolved
api/test/e2enew/route/route_with_methods_test.go Outdated Show resolved Hide resolved
api/test/e2enew/route/route_with_methods_test.go Outdated Show resolved Hide resolved
api/test/e2enew/route/route_with_methods_test.go Outdated Show resolved Hide resolved
@Jaycean
Copy link
Member

Jaycean commented Mar 29, 2021

Okay, @Jaycean, I'm opening an issue.
See the ci is also failing with a 404 for that particular test.
image

@bisakhmondal
This error is that the URI created by APISIX is forwarded to the upstream, and the corresponding interface is also required. I checked the conf in the upstream image and found that the supporting interface is hello hello1 hello_ You can change update URI to hello_

File path:In the docker container: johz/upstream
/usr/local/openresty/nginx/conf/lua/server.lua

function _M.hello()
    ngx.say("hello world")
end

function _M.hello1()
    ngx.say("hello1 world")
end

function _M.hello_()
    ngx.say("hello world")
end

hellohello is not supported in the upstream interface, so the error returned is 404.

function _M.go()
    local action = string.sub(ngx.var.uri, 2)
    action = string.gsub(action, "[/\\.]", "_")
    if not action or not _M[action] then
        return ngx.exit(404)
    end

    inject_headers()
    return _M[action]()
end

@bisakhmondal
Copy link
Member Author

Wonderful @Jaycean❤️. Thank you for the elaborated information.
So the whole time the 404 was thrown by upstream itself😶.

@Jaycean
Copy link
Member

Jaycean commented Mar 29, 2021

Wonderful @Jaycean. Thank you for the elaborated information.
So the whole time the 404 was thrown by upstream itself.

Yes, after all, this upstream is just for testing

@bisakhmondal
Copy link
Member Author

Yes, after all, this upstream is just for testing

No issues. Totally agree with you :)

@juzhiyuan juzhiyuan merged commit e25afe3 into apache:master Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants