Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

Add tests to compile generated code #275

Merged
merged 37 commits into from
Dec 22, 2018
Merged

Conversation

Weakky
Copy link
Contributor

@Weakky Weakky commented Nov 9, 2018

Tries to compile every tests to make sure there are no errors

  • Typescript
  • Flow

Fixes #69

@Weakky Weakky force-pushed the tests/compile-templates branch from 11865a6 to 606fe1d Compare November 9, 2018 18:17
@Weakky Weakky changed the title [WIP] Add test to compile templates [WIP] Add tests to compile templates Nov 9, 2018
@Weakky Weakky force-pushed the tests/compile-templates branch 6 times, most recently from 32033d2 to 97aa601 Compare November 9, 2018 19:59
@Weakky Weakky changed the title [WIP] Add tests to compile templates [WIP] Add tests to compile generated code Nov 9, 2018
@Weakky Weakky force-pushed the tests/compile-templates branch from 97aa601 to d184cb4 Compare November 10, 2018 10:23
@Weakky Weakky force-pushed the tests/compile-templates branch from d184cb4 to 5505044 Compare November 10, 2018 10:25
@Weakky Weakky force-pushed the tests/compile-templates branch from 07af646 to e76fb8e Compare November 10, 2018 12:41
@jasonkuhrt jasonkuhrt self-assigned this Dec 13, 2018
@jasonkuhrt
Copy link
Member

jasonkuhrt commented Dec 13, 2018

Started onboarding myself into this pr tonight. A small dep change to fix a yarn warning allowed the tests to pass locally. They pass on ci too, but then (it seems) jest never exits and so the ci times out. Aside from ramping up, a difficulty here is the lack of feedback. To help narrow the issue, I may try to incrementally temporally eliminate test code until the ci problem goes away.

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Dec 14, 2018

I didn't observe carefully before. Looking again at the tests running on c, I noticed that the two "large-schema" tests are missing from the output, e.g. from local:

 PASS  src/tests/flow/large-schema.test.ts (8.686s)
 PASS  src/tests/typescript/large-schema.test.ts (13.524s)

When I isolate the large-schema tests they run ok though (build link).

Looking at past recent failures it seems there is a random-like failure among "basic" and "large" schema tests across both TS and FT.

I would ssh into the build container but I don't have permission:

image

@jasonkuhrt jasonkuhrt force-pushed the tests/compile-templates branch from 32cbd52 to eedf697 Compare December 14, 2018 02:09
@jasonkuhrt
Copy link
Member

jest --runInBand is one solution. It increases the test time however. Will continue looking for a root cause.

@jasonkuhrt jasonkuhrt force-pushed the tests/compile-templates branch from b5b587f to 353b212 Compare December 14, 2018 02:32
@jasonkuhrt
Copy link
Member

Eventually figured out that the disk contention was not the root cause (probably not implicated at all). There are quite a few existing issues on the inerwebs about cci/travis not handling jest auto-cpu-count detection well. Capping worker count on CI worked here too.

I found one flow test that was commented out and needed finishing, default name. What I found was that flow wouldn't compile because (not type name mismatch):

export interface Mutation_AddMemberData {
  email: string;
  projects: string[];
}

export interface Mutation_Args_AddMember {
  data: AddMemberData;
}

I now append prefixes to the types of field arguments (in the above case... Mutation_) to comply with the pattern in our snapshots. I do not really understand the flow strategy, so, I'm not 100% confident with my commit here; in a nutshell: every field arg type gets a prefix except enum, input, object (which correspond to at least in some tests to GQL ID type).

@@ -2,7 +2,11 @@ import * as os from 'os'
import * as prettier from 'prettier'
Copy link
Member

Choose a reason for hiding this comment

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

This module contains bug-fix code in response to the new compilation step we do.

Copy link
Contributor

@timsuchanek timsuchanek left a comment

Choose a reason for hiding this comment

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

Thanks a lot both @Weakky and @jasonkuhrt for all this work! This will add soo much stability to our releases. Besides the two lets, the PR looks amazing 🙌

packages/graphqlgen/src/tests/generation.ts Outdated Show resolved Hide resolved
packages/graphqlgen/src/tests/generation.ts Outdated Show resolved Hide resolved
@jasonkuhrt jasonkuhrt changed the title [WIP] Add tests to compile generated code Add tests to compile generated code Dec 18, 2018
@jasonkuhrt
Copy link
Member

jasonkuhrt commented Dec 19, 2018

@timsuchanek let me know if there are any other blocking issues in this PR from your point of view.

@jasonkuhrt
Copy link
Member

@timsuchanek given that you seemed pretty satisfied already and I've fixed the two style issues you noted, going to merge, keep things rolling. And these tests like you said will start helping immediately : )

@jasonkuhrt jasonkuhrt merged commit 97fa5a6 into master Dec 22, 2018
@jasonkuhrt jasonkuhrt deleted the tests/compile-templates branch December 22, 2018 05:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants