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

ci: check scripts #922

Merged
merged 9 commits into from
May 5, 2022
Merged

ci: check scripts #922

merged 9 commits into from
May 5, 2022

Conversation

Shinigami92
Copy link
Member

closes #917

@Shinigami92 Shinigami92 added the c: chore PR that doesn't affect the runtime behavior label May 4, 2022
@Shinigami92 Shinigami92 self-assigned this May 4, 2022
@Shinigami92 Shinigami92 added p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug labels May 4, 2022
@Shinigami92 Shinigami92 force-pushed the 917-ci-check-scripts branch from fdac641 to 518b3fc Compare May 4, 2022 10:53
@codecov
Copy link

codecov bot commented May 4, 2022

Codecov Report

Merging #922 (eb3acd3) into main (3ea64ce) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #922      +/-   ##
==========================================
- Coverage   99.66%   99.66%   -0.01%     
==========================================
  Files        1957     1957              
  Lines      209797   209797              
  Branches      886      885       -1     
==========================================
- Hits       209087   209084       -3     
- Misses        690      693       +3     
  Partials       20       20              
Impacted Files Coverage Δ
src/modules/finance/index.ts 99.31% <0.00%> (-0.69%) ⬇️

@Shinigami92
Copy link
Member Author

TS-Errors are reported successfully:

@Shinigami92 Shinigami92 requested a review from ST-DDT May 4, 2022 10:59
@Shinigami92
Copy link
Member Author

awaiting merge of #915

@Shinigami92 Shinigami92 added the s: on hold Blocked by something or frozen to avoid conflicts label May 4, 2022
@Shinigami92
Copy link
Member Author

Shinigami92 commented May 4, 2022

I don't know why but the results differs when I run
pnpm run ts-check:scripts vs tsc --noEmit --esModuleInterop --skipLibCheck scripts/**/*.ts manually 🤔

Edit1:

It has something todo with the glob pattern which is not fully resolved in CI

Edit2:

Yeah that looks like a bug, because pnpm run ts-check:tests | grep 'scripts/apidoc/utils.ts' while using --explainFiles and test/**/**/*.ts is the only way I reach the requested file. When using test/**/*.ts it does not check the utils file.
I assume I need to resolve the globs manually 😕

Edit3:

I use config files now, as these seems to work 🤷

@ST-DDT
Copy link
Member

ST-DDT commented May 4, 2022

It has something todo with the glob pattern which is not fully resolved in CI

Depending on OSes you have to manually enable glob patterns, but I like the configs more anyway.

https://unix.stackexchange.com/questions/388695/how-to-turn-globbing-on-and-off

@ST-DDT
Copy link
Member

ST-DDT commented May 4, 2022

This looks like the perfect place for using extends.

@Shinigami92
Copy link
Member Author

This looks like the perfect place for using extends.

I didn't use it for tsconfig.json as I'm to afraid to break something in build process.
Also I needed to reverse some settings explicit set for scripts and tests and therefore the file is nearly same length 🤷

@ST-DDT ST-DDT removed the s: on hold Blocked by something or frozen to avoid conflicts label May 4, 2022
@ST-DDT
Copy link
Member

ST-DDT commented May 4, 2022

#915 is merged.

package.json Show resolved Hide resolved
@Shinigami92 Shinigami92 marked this pull request as ready for review May 5, 2022 08:39
@Shinigami92 Shinigami92 requested a review from a team as a code owner May 5, 2022 08:39
@Shinigami92 Shinigami92 requested a review from a team May 5, 2022 08:39
@ST-DDT ST-DDT merged commit 0b2863e into main May 5, 2022
@ST-DDT ST-DDT deleted the 917-ci-check-scripts branch May 5, 2022 12:11
@xDivisionByZerox xDivisionByZerox added the c: infra Changes to our infrastructure or project setup label Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: chore PR that doesn't affect the runtime behavior c: infra Changes to our infrastructure or project setup p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

ci: check whether scripts still compile
4 participants