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 refactor: Serverless tests, updated new-game syntax #3842

Merged
merged 4 commits into from
Sep 28, 2018

Conversation

NoahTheDuke
Copy link
Collaborator

@NoahTheDuke NoahTheDuke commented Aug 13, 2018

This is a re-up of the first part of #3782. I pulled out two of the smallest meaningful changes, and once it's merged will open the other two PRs that encompass the card definitions and the card tests.

Changes:

  • Moved the json data to individual edn files in the data/cards folder.
  • Rewrote all card-loading logic in testing to read from these files.
  • Rewrote test creation logic: now you can start the game with cards already in hand or in the discard, you can set starting credits to be different, you can leave out a side if they're gonna be defaulted, and you can set a given side's identity without using a new function.
  • Changed all card-watching and reloading logic to use hawk, which is legit straight magic.
  • Removed all fixtures as they didn't actually do what I wanted them to.
  • Added eftest as a dependency and plugin. It's a stupid fast test runner, running tests in parallel. At the time of writing, it handles all 871 tests in 20 seconds.

@NoahTheDuke NoahTheDuke requested a review from Saintis August 15, 2018 12:14
@Saintis
Copy link
Collaborator

Saintis commented Aug 15, 2018

Looks good! I like the new test setup, feels more clojure and makes organising deck and hand easier.

Is the deck sorted or randomised for the Corp, or does setting hand / deck suppress mandatory draw? Just thinking about how you could make sure you only have the specified cards in hand during corp turn.

What is the reasoning behind including the card edn files in the repository?

That hawk thing looks magical and I want it now.

@NoahTheDuke
Copy link
Collaborator Author

NoahTheDuke commented Aug 15, 2018

Mandatory draw: Moving cards to hand or trash happens after mandatory draw/start of turn, so if specified, :hand cards will will be exactly what is listed. Additionally, as I didn't explicitly note it (fixed), the deck isn't just what's listed in :deck, but also what's listed in :hand and :discard. Meaning you don't have to double-specify cards as being in both deck and hand for them to appear in hand (unless you want them in both to start).

EDN files: I included them for future proofing and the "serverless tests". Instead of relying on NRDB to be our source of truth, we can update the files as necessary when changes come through Nisei or from PRs to netrunner-json. This also handles any issues with CircleCI not being able to contact NRDB when it goes down or changes in ways we don't expect.

I might previously have balked at doing this, but with no new cards coming (at least not for a long time), it seems best to explicitly allow for custom card creation through adding new files or editing existing ones. This also means that if/when we make changes to the card schema (which is something I plan on doing at some point), we don't have to rely on folks re-running lein fetch, we can just push the changes and let them pull the updated files.

Hawk: Yeah, it's way better than any of the solutions I'd created. It's the backing for a future change to card definition reloading that'll come with the individual card definition files PR.

@Saintis
Copy link
Collaborator

Saintis commented Aug 15, 2018

Some good points with regards to the edn files. What if we create our own "version" of netrunner-json as netrunner-edn that we can pull from using git? Or even change to pulling netrunner-json directly instead of going via NRDB?

@NoahTheDuke
Copy link
Collaborator Author

Oh that'd be very clever: maintain our own version, converted for our needs? I'd definitely be into that. I know the dudes over at ringteki considered this for a hot minute, but I believe they considered it too much work in the long run (as they only really need it during spoiler season).

I'll experiment, see if it's worth it to do that!

@nealterrell nealterrell merged commit c21b2ba into mtgred:master Sep 28, 2018
@NoahTheDuke NoahTheDuke deleted the test-refactor branch September 28, 2018 14:16
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.

3 participants