[nodejs] enable fastify cookies tests #4803
Conversation
IlyasShabi
force-pushed
the
ishabi/fastify-cookies
branch
from
bf8ef18 to
4fe533b
Compare
IlyasShabi
force-pushed
the
ishabi/fastify-cookies
branch
from
4fe533b to
e3eb64f
Compare
IlyasShabi
changed the title
tests/appsec/test_fingerprinting.py
Outdated
| @@ -192,6 +187,7 @@ def setup_session_non_blocking(self): | |||
| self.cookies = self.r_create_session.cookies | |||
| self.r_user = weblog.get("/user_login_success_event", cookies=self.cookies) | |||
|
|
|||
| @missing_feature(context.weblog_variant == "fastify", reason="session fingerprint not supported yet") | |||
There was a problem hiding this comment.
the whole class should fail not jusr this test no ?
There was a problem hiding this comment.
test_session_blocking is passing 🤔
There was a problem hiding this comment.
did you add a session middleware to fastify weblog ? if not, it's a problem in the test
There was a problem hiding this comment.
There was a problem hiding this comment.
even then, it doesn't make sense that the "non_blocking" test is skipped, "but the "blocking" one passes. If a feature works with blocking, it should work without blocking
tests/appsec/test_fingerprinting.py
Outdated
| @@ -192,6 +187,7 @@ def setup_session_non_blocking(self): | |||
| self.cookies = self.r_create_session.cookies | |||
| self.r_user = weblog.get("/user_login_success_event", cookies=self.cookies) | |||
|
|
|||
| @missing_feature(context.weblog_variant == "fastify", reason="session fingerprint not supported yet") | |||
| def test_session_non_blocking(self): | |||
| assert self.r_create_session.status_code == 200 | |||
There was a problem hiding this comment.
try without this line see if the test pass ?
| assert self.r_create_session.status_code == 200 | |
| # assert self.r_create_session.status_code == 200 |
There was a problem hiding this comment.
It's working yes
There was a problem hiding this comment.
well, the session fingerprinting blocking test doesn't have this check, so why would the nonblocking one have it. Either they should both have it, or neither. I think it's safe to remove ?
There was a problem hiding this comment.
It's a test change, so talk about it with other lib engs
There was a problem hiding this comment.
well, I fixed the test by modifying the endpoint behaviour and I added a todo comment.
I think this should not be a blocker for this PR since we still miss session management with fastify, however we can ask @Anilm3 about this since he implemented this test https://github.com/DataDog/system-tests/pull/4382/files#diff-c4d709ffdb6e4493d3ee3f20c624c1e5ec0fa3ee4ad6ad6bb435e3c0ad56e1d1R100
Motivation
Enable fastify cookies tests
Changes
Workflow
codeownersfile quickly.🚀 Once your PR is reviewed, you can merge it!
🛟 #apm-shared-testing 🛟
Reviewer checklist
[<language>], double-check that only<language>is impacted by the changebuild-XXX-imagelabel is present