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

Test adjustments and improvements. #425

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Test adjustments and improvements. #425

wants to merge 1 commit into from

Conversation

Tzrlk
Copy link

@Tzrlk Tzrlk commented Apr 15, 2023

  • Reducing code duplication
  • Using import destructuring.
  • Ignoring integration test file output.
  • Changing package 'test' script to not use hashbang execution.
  • Explicitly requiring jake rather than relying on global import
  • Fixing integration tests to run on Windows.

@Tzrlk
Copy link
Author

Tzrlk commented Apr 15, 2023

I'm aware this set of changes is less surgical than it could be, so if there's anything you'd like me to pull back on, please let me know.

I really wanted to be able to get the tests running properly in my windows environment before I started rewriting more things.

@Tzrlk
Copy link
Author

Tzrlk commented Apr 15, 2023

I also note that the travis build specifies node 8, 10, and 12. These changes will definitely not pass those builds. I might look into improving the test runner to execute the jake cli against different nodejs versions, so compatibility with older runtimes can be maintained with transpiling and/or conditional shims.

@mde
Copy link
Contributor

mde commented Apr 16, 2023

This looks amazing! However, I noticed a bunch of semicolon-less lines in your changeset, and reaalized the following:

  • I never added a lint task in the Jakefile (it only existed in the package.json)
  • ESLint's defaults seem to have shifted to allow semicolon-less style, so I needed to an explicit rule

I have fixed both of these issues. The conflicts in your PR are doubtless just collisions where eslint --fix added semis, and the one strange added comma in that long regex that you already identified.

I am not super concerned about the older versions of Node. Jake has been around since Node v2, and sometimes things in the codebase take a while to catch up to reality.

* Reducing code duplication
* Using import destructuring.
* Ignoring integration test file output.
* Changing package 'test' script to not use hashbang execution.
* Explicitly requiring jake rather than relying on global import
* Fixing integration tests to run on Windows.
* Fixing no-sparse-arrays eslint error.
* Fixing assertion param ordering
* Making test tmp dir deletion/creation easier.
* Fixing windows jake batch script.
* Ensuring `jake lint` passes.
* Removed some unused params.
@Tzrlk
Copy link
Author

Tzrlk commented Apr 20, 2023

I'm super happy I got a positive response to this; thanks a ton.
Still have to do something with the build, but it's all clear locally now.

Do you have a preference for Travis? Or would Github actions do just as well?
I'll throw some branches together with various changes to make things a bit easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants