Skip to content
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

Automate E2E testing #852

Closed
k2snowman69 opened this issue Aug 14, 2019 · 2 comments
Closed

Automate E2E testing #852

k2snowman69 opened this issue Aug 14, 2019 · 2 comments
Labels
package: typescript-estree Issues related to @typescript-eslint/typescript-estree tests anything to do with testing

Comments

@k2snowman69
Copy link

k2snowman69 commented Aug 14, 2019

I saw in PR #703 there was a request

Thanks a lot for the detailed write up @nathanmarks. The main priority I would have before merging this is ensuring that it is not going to cause any regressions in Prettier.
Please could you try applying the changes in your PR to Prettier to double check? From memory there is a fairly comprehensive set of tests for comments in JSX over there...

If this is a requirement for PRing, it may make more sense to create a CI build to test this scenario. If you decide this is a good idea, the dockerfile would look something like this (sorry if it's not 100%, copying from a personal project):

## Build a node application
from node:8.11.1

## typescript-eslint creation, build and test
WORKDIR /ts-eslint
COPY . .

RUN npm install --unsafe-perm
RUN npm run test

## prettier
RUN echo "### prettier"
WORKDIR /prettier
RUN git clone https://github.com/prettier/prettier .
RUN npm install --unsafe-perm
RUN npm link /ts-eslint
RUN npm run build
RUN npm run test
@k2snowman69 k2snowman69 added package: typescript-estree Issues related to @typescript-eslint/typescript-estree triage Waiting for maintainers to take a look labels Aug 14, 2019
@bradzacher bradzacher added tests anything to do with testing and removed triage Waiting for maintainers to take a look labels Aug 14, 2019
@bradzacher
Copy link
Member

I think that this was a unique situation.
A few months ago, a contributor added a seemingly innocuous change to the way JSX was parsed. This caused a bug which I believe meant that the comment detection was overly aggressive (it flagged urls within the JSX text as comments).

Another contributor then came in from the perspective of a prettier user, and put in a fix. This change caused a bug in which the comments were now ignored because they were not added to the AST (but this fixed the bugs from prettier's POV).

So with #703 we just wanted to make sure that this 3rd fix actually fixed everything on both sides of the fence.

Considering that there is a dependency on us from prettier, we should probably make sure that we don't break anything for them.
I don't see the problem with adding integration tests to ensure that we don't accidentally break them again.

@JamesHenry
Copy link
Member

Yes, exactly what Brad says regarding the context.

In terms of what to do long term, we could set up a nightly job for it, but actually making it part of the PR process would add too much overhead on our side I think.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: typescript-estree Issues related to @typescript-eslint/typescript-estree tests anything to do with testing
Projects
None yet
Development

No branches or pull requests

3 participants