-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Multiple enhancements to typescript fetch generator #145
Conversation
It seems it doesn't build into CI but I don't understand why... Any idea guys? |
I think shippable fails because the kotlin-server sample is not up-to-date (it fails in the ensure-up-to-date.sh file because regenerating the kotlin-server creates changes in git). Travis CI (both) fail because typescript-fetch tests are failing: |
@TiFu How can I run the test and update them? I don't think there were test before when I was working in swagger-codegen :) |
You should be able to run the tests locally with
You can also check the Travis log for details: https://travis-ci.org/OpenAPITools/openapi-generator/builds/383322883 |
@JFCote as discussed, the issue probably has to do with TypeScript version update to 2.4. We need to update the Petstore integration tests accordingly. cc the technical committee to see if anyone has cycle to fix the tests for TS Fetch petstore client. @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) |
@wing328 I can modify the tests if someone explain me where they are and what they do. Are they unit tests? Do they test the output? Any documentation I can check to update the tests of a generator? |
The tests can be found in https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/typescript-fetch/tests/default Run CI errror log: https://api.travis-ci.org/v3/job/391347920/log.txt |
@wing328
I don't understand the error with npm since I'm able to run |
@JFCote what about running the following commands manually in that folder?
|
@wing328 Will try this today and let you know! |
@wing328 Didn't have time today, will try in the upcoming days! |
I just checked out this PR locally and both |
@msiemens 3 of the 5 CI doesn't seems to pass. I will try to resolve the conflict today and see. |
# Conflicts: # samples/client/petstore/typescript-fetch/builds/default/.openapi-generator/VERSION # samples/client/petstore/typescript-fetch/builds/es6-target/.openapi-generator/VERSION # samples/client/petstore/typescript-fetch/builds/with-interfaces/.openapi-generator/VERSION # samples/client/petstore/typescript-fetch/builds/with-npm-version/.openapi-generator/VERSION
@msiemens I just fixed the minor conflict with the master with a merge. Let's see how it goes with the CI. |
Thanks! Seems to still be failing (at least on Travis, the others are still running). It's strange that I can't reproduce this locally… |
@msiemens It seems something is still not working... Don't understand exactly what it is. There is sooooooo much information in the logs I lose myself. |
did you re-generate the typescript fetch samples after the merge? |
I forgot to actually change the branch when testing on my last try 🙈 Now with the correct branch I can reproduce this locally and it seems like the error comes from changing import * as portableFetch from "portable-fetch"; to import portableFetch from "portable-fetch"; They are semantically different in TypeScript, the first one imports all exports from openapi-generator/modules/openapi-generator/src/main/resources/typescript-fetch/api.mustache Line 6 in 86c3bc0
|
@msiemens Can you please write down the exact command you are running to reproduce the error? I'm very new to this concept of testing with typescript / npm and I'm not sure what command to launch and in which folder I need to be. As soon as I'm able to reproduce the error on my local machine, I will have if fixed very quickly. |
@JFCote I just ran Alternatively, you could run the test setup and execution manually by calling |
@msiemens I am unable to debug. When I run I get this result, with or without the changes you suggested. This is with stuff commented. If I uncomment all test, then they all fail. It seems mvn plugin are not working very well with npm or something... Any ideas ?
When I run `npm install" manually in this folder, I have no problem at all. But when I run the suggested manual call:
I would like to help but making these test work seems to be VERY complicated. @wing328 Any ideas why test are not working on my local machine? |
Are you sure, you're calling this from |
@msiemens Oups, you are right! For example:
Is this the "expected" behvior or the error we are talking about? |
Yes, this is the error :) If you apply the changes mentioned in #145 (comment), the errors should disappear (after rebuilding the sample client using |
@msiemens Ok I think we have a problem, or maybe my computer is giving me false flags. When I checkout the Master (without my changes) and do this:
I still receive the errors like this:
So unless I'm missing something, it seems the test weren't passing even in the master, so it's not related to my changes. I'm calling for the help of the Typescript technical commitee: @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) |
I just ran everything as you described it on master, and all tests pass without errors. Also, recent commits on master pass all tests, for example https://travis-ci.org/OpenAPITools/openapi-generator/builds/405202474 for 9056c1e |
After your message, I deleted EVERYTHING and started from scratch and now you are right, it work in master... So much time wasted... There must have been something odd in one of the node_modules... I'll come back to you later today with a fix Thanks |
No worries, stuff like that happens to everyone 🙂 It took me more than one try figure everything out, too |
Now it seems to crash because some "pom.xml" files disappeared from the samples. But like I said, I deleted all the fetch samples and re-generated them all... So if the pom.xml wasn't re-generated, maybe this check is not good in the test? @msiemens , any advice? |
I don't know why the git checkout HEAD~1 -- samples/client/petstore/typescript-fetch/builds/default/pom.xml samples/client/petstore/typescript-fetch/builds/es6-target/pom.xml samples/client/petstore/typescript-fetch/builds/with-npm-version/pom.xml to retrieve the files back and commit the changes. |
Yay, everything is finally passing now 🎉 |
Merged since it has been reviewed by a lot of people :) Thanks everyone. |
Hi, The issue was fixed in this branch correct? kr |
@Derryrover can you open a separate issue and add details about the error message? there are many reasons why |
Hi macjohny |
PR checklist
./bin/
to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh
and./bin/security/{LANG}-petstore.sh
if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\
.master
.Description of the PR
@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01)
That is a copy from my own PR in swagger-api/swagger-codegen#7914 that has never been merged. @wing328 ask me to redo it here