search

raulvillares / 3sacrowd

Tic-Tac-Toe puzzle using plain javascript :video_game:
https://raulvillares.github.io/3sacrowd/index.html
35 stars 69 forks source link

Re-configured tests #67

Closed OleDakotaJoe closed 3 years ago

OleDakotaJoe commented 3 years ago

Summary

  1. There was an error when trying to run tests using the provided command in the README file. --Due to an error with the 'import' keyword the tests needed to be refactored to use the require() method.
  2. Set up tests to be able to be run using an npm script, making running any one, or all, tests easier.
  3. Since the tests are now included in the package.json file, there was no longer any need for the infoTests.js file, so it was deleted.
  4. Updated the readme to include simpler steps for cloning, installing dependencies, running tests, and guidelines for contributing more tests.

Other Information

The issue linked to this can PR can be found here. A snippet of the errors is shown in the original issue.

titivermeesch commented 3 years ago

I'm wondering if it's a good idea to convert them back to require. It's a pretty old syntax and import would be much cleaner. It's probably better to make the tests work with import

OleDakotaJoe commented 3 years ago

Hey good point. I will do some research and see if I can get them working, but so far I have only been able to accomplish this by doing one of two things:

  1. Settign the type=module int the <script src=foo type= module> tag
  2. settign the file type extension to fileName.mjs

Can you advise on another method?

OleDakotaJoe commented 3 years ago

Update

I cannot find any method of implementing these tests as is and still using requirejs , without adding another dependency, or refactoring the entire project to remove requirejs, but then adding a global state dependency.

As is, the tests are completely inoperable, as requirejs is a 10 year old project, and came out long before es2015.

Do you have any suggestions on how to move forward?

titivermeesch commented 3 years ago

requirejs can stay, it's just the require import syntax that should not be used.

I checked and you could just import requirejs with the import keyword, and put type=module inside package.json. That is working for me (also check that you are using an updated node version)

OleDakotaJoe commented 3 years ago

Refactored

Refactored PR to include newer ES6 syntax import rather than older require() syntax.

Thanks for the tip.