Skip to content

Add starter kit to user tests#19934

Merged
sandersn merged 5 commits into
masterfrom
add-starter-kit-to-user-tests
Nov 13, 2017
Merged

Add starter kit to user tests#19934
sandersn merged 5 commits into
masterfrom
add-starter-kit-to-user-tests

Conversation

@sandersn
Copy link
Copy Markdown
Member

Add starter kit repos to user tests. This works by looking for a test.json file in the test directory. If one is present, then the runner updates submodules, the changes the working directory to the submodule's subdirectory.

The types field from test.json is used to prevent errors from compiling inside of the Typescript project. And the --noEmit flag is added to everything to allow empty --types to work, in addition to speeding up test times.

If there is a test.json in the directory, it expects to find a
submodule in the directory. The submodule should have the same name as
the directory itself. test.json contains a list of global types that
need to be available, or the empty list if none.
@sandersn sandersn requested a review from weswigham November 10, 2017 23:38
@sandersn
Copy link
Copy Markdown
Member Author

I did not baseline React, React-Native or WeChat, even though they currently fail, because I have a simple change to tsconfig for each.

Even if we need to fully upgrade each repo to the latest versions of their respective libraries, I think we can do that instead fairly quickly.

Comment thread src/harness/externalCompileRunner.ts Outdated
if (update.status !== 0) throw new Error(`git submodule update for ${directoryName} failed!`);

const config = JSON.parse(fs.readFileSync(path.join(cwd, "test.json"), { encoding: "utf8" })) as UserConfig;
ts.Debug.assert(!!config.types, "Git is the only reason for using test.json right now");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"Git is the only reason for using test.json right now"? That's not what's being checked here, so it's odd. I think "Types field must be present" would be sufficient. And likely not need to change in the future.

Copy link
Copy Markdown
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Seems good.

Inconsequential question:
The tests update the submodules regularly, which is good; do we want a bot to commit those updated submodules when they change without consequence to the baselines? I think they'll show up in git status (not sure) each time they're changed, if that affects your response.

@sandersn
Copy link
Copy Markdown
Member Author

That's an interesting question that we should discuss in person. I'm not sure which you are suggesting:

  1. I don't expect people to run this locally except when it fails. This is a good time to commit changes since (1) either the test needs to be updated or (2) something is wrong with Typescript itself. Either way, we will be committing changes.
  2. Since cron is running this daily, it could commit submodule updates if the daily run passes.

(1) seems more desirable to me, and easier to write, since (I guess) it's just an additional call to git add after updating the submodule. And we don't have to worry about a cron job going mad with power and committing 1,000,000 erroneous/insane commits.

@sandersn sandersn merged commit 7771d0c into master Nov 13, 2017
@sandersn sandersn deleted the add-starter-kit-to-user-tests branch November 13, 2017 16:54
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
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.

2 participants