Open torbencarstens opened 10 months ago
I'm sorry to say that I'm still not entirely sure how https://github.com/dom96/jester/issues/247#issuecomment-1722790239:
My suggestion was for a way to do it without changing jester-code, but just hooking in on available fields = the .url
My mistake - I was sure, that there were a .url
field which was public available, so you could roll you own procs on top. That would also have allowed users to implement their own query-collector, whether it is a Table[string, seq[string]]
or something else.
This is when httpbeast
is used. The httpbeast#head
includes a fix, but it must be published with a new tag (https://github.com/dom96/httpbeast/issues/91).
Alright, thanks for the info. I guess the addition of this CI version can be done at a later point then.
Regarding the CI on windows, the exact error message is (nim: 1.6.14)
C:\Users\{user}\.nimble\pkgs\asynctools-#0e6bdc3ed5bae8c7cc9\asynctools\asyncpipe.nim(260, 16) Error: undeclared identifier: 'PCustomOverlapped'
I found the following issue in asynctools: https://github.com/cheatfate/asynctools/issues/31 which seems to explain this.
How do you want to proceed with this?
Aside from these 2 issues, is there anything left which has to be addressed (additional reviews/changes/documentation/examples/...)?
I guess at least the CI workflow still has to be approved and we need to test this on the mac pipeline (needs a decision on how the current windows bug should be handled).
The .gitignore
has two new files, which might be due to your editor preference - they can be removed.
For the asynctools
I dont know - I'm not running windows... sorry.
I like the PR and your additional tests 👍. But, I'm just a contributor to the repo, so it must be @dom96 who can give you the directions for the next step.
Alright, thanks for your help. I've reverted the .gitignore
commit.
I'll wait for some input from dom then.
Note about the Windows CI failures, I've fixed the asynctools
issues locally but the tests still fail (explicitly import asyncpty
and use asyncpty.PCustomOverlapped
directly in asyncipc
/asyncpipe
).
I ran these on the current master to ensure that none of my changes has any impact on this. (I've ran mine as well and they're all [OK]
).
The following test suites fail (I haven't looked into why this is):
Note that the kill ${PID}
message is a bit misleading due to the finally
block error being directly underneath, the actual kill
output comes from here https://github.com/dom96/httpbeast/blob/17e322ba68b264e9f4ec755e6946c64242d289ad/tests/tester.nim#L32 and is thrown in the allTest(useStdLib=true)
testsuite when startServer
is called.
I guess the impact from the Windows CI failing should be ignored in this case since it fails due to conditions which are already on master, this should probably be fixed separately.
nim c --run tests/tester.nim
(nim 1.6.14)Jester 0.6.0
[FAILED] test_file.txt [OK] detects attempts to read parent dirs [Suite] extends [OK] simple [OK] params [OK] separate module [OK] external regex [OK] regex path prefix escaped [Suite] error [OK] exception [OK] HttpCode handling [OK] `pass` in error handler C:\Users\{user}\projects\jester\tests\tester.nim(220, 32): Check failed: (waitFor resp.body) == "404 not found!!!" (waitFor resp.body) wasJester 0.6.0
[FAILED] custom 404 [Suite] before/after [OK] before - halt [OK] before - unaffected [OK] before - global C:\Users\{user}\projects\jester\tests\tester.nim(237, 22): Check failed: resp.code == Http404 resp.code was 400 Bad Request Http404 was 404 Not Found [FAILED] before - 404 [OK] after - added kill: 17728: No such process kill: 17728: No such process C:\Users\{user}\projects\jester\tests\tester.nim(296) tester C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\system\assertions.nim(38) failedAssertImpl C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\system\assertions.nim(28) raiseAssert C:\Users\{user}\.choosenim\toolchains\nim-1.6.14\lib\system\fatal.nim(54) sysFatal Error: unhandled exception: C:\Users\{user}\projects\jester\tests\tester.nim(296, 14) `execCmd("kill -15 " & $serverProcess.processID()) == QuitSuccess` [AssertionDefect] Error: execution of an external program failed: 'C:\Users\{user}\projects\jester\tests\tester.exe ' ```
Closes #247
I've implemented an example solution (new function) so that we can discuss further based on this.
I've opened this as a PR since the actual implementation discussion is not really relevant for the issue itself.
I'm sorry to say that I'm still not entirely sure how the following would work:
I'm still fairly new to nim (<1000 lines) so forgive me if I'm missing something obvious here, the only options I see to implement this are
dotOperators
)url
property (new type?) toRequest
which supports this feature (this modifies code even further though and would be inconsistent with the current implementation)proc
to a different implementation when explicitely called by the user (or the library)httpbeast
and the standard library and expose this via jester (also needs code changes since theNativeRequest
isn't exposed to the user) (also unlikely to be accepted)That being said, this should probably be a separate function in any case which can be used by any of the other options if another shortcut/accessor is implemented for this.
I'm not entirely happy with the name (feel free to suggest others) but I hope it's at least clear enough on what it does.
I've removed the explicit call to
decodeUrl(value)
(in bothparams
andparamValuesAsSeq
) sincedecodeData
(decodeQuery
internally) does the decoding for us already.parseUrlQuery
I've also updated the
params
function to not useparseUrlQuery
anymore since this has been marked as deprecated in any case.With this change the 2 function operate essentially the same exact way (excluding the return type).
I haven't removed it since it's exported and might be used by users.
Tests
I've also implemented some tests for both
params
andparamValuesAsSeq
(exactly the same tests with different results).I tested this against the old version of
params
and the new version.CI
os
Currently the CI:
versions
CI succeeds for nim 1.4.8 and 1.6.14, for nim 2 the CI throws a
SIGSEGV: Illegal storage access. (Attempt to read from nil?)
.I've observed this behaviour with jester before and either running nim with
--mm:refc
or importingstd/segfaults
fixed this (which is very frustrating).I've only tested running with
--mm:refc
which didn't fix it in the CI, I didn't want to importstd/segfaults
in all tests only for the CI to work since this is obviously a bug somewhere.I haven't looked into this any closer though, everything (tests + jester itself) run fine for me with nim 2.0 (locally and in docker).
Comments on things I've updated which aren't directly related to the issue. I hope including this here is fine with you, otherwise I don't mind pulling these changes out and opening a separate PR with them.
Exclude testing
tests/nim-in-action-code
fornim >= 2.0
nim-in-action-code isn't trying to be compatible with nim 2 (as far as I can tell).
The compilation currently fails because
db_sqlite
isn't available anymore (needs animble install db_connector
and a change of import, i.e. ->db_connector/db_sqlite
Update the CI
Basically this #319, I didn't want a broken CI when submitting a PR.
Update
nimble test
Up to now the user had to execute some steps themselves (e.g. refresh/install),
nimble test
should now work without any further input from the user after cloning the repository..gitignore
I've developed with intellij so I've added the common ignores for this IDEhas been reverted (see https://github.com/dom96/jester/pull/323#issuecomment-1725868468)