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

Move partials to macros and tests #59

Merged
merged 2 commits into from
Feb 2, 2024
Merged

Move partials to macros and tests #59

merged 2 commits into from
Feb 2, 2024

Conversation

mkly
Copy link
Member

@mkly mkly commented Feb 2, 2024

  • Move partials to macros for testability
  • Add tests for components and pages

* Move partials to macros for testability
* Add tests for components and pages
Copy link

github-actions bot commented Feb 2, 2024

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

@mkly
Copy link
Member Author

mkly commented Feb 2, 2024

Just some quick tests/shuffling around to make it a bit quicker for me to dig into this stuff.

@mkly mkly requested a review from dhosterman February 2, 2024 00:12
@mkly mkly requested a review from wpietri February 2, 2024 00:13
Copy link
Contributor

@wpietri wpietri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but could you say a bit more about the kind of programmer error that's motivating those particular new tests?

@mkly
Copy link
Member Author

mkly commented Feb 2, 2024

Looks good to me, but could you say a bit more about the kind of programmer error that's motivating those particular new tests?

I find it a bit challenging as it stands to know where things are changing where edits are made. There is global scope and components and I'm trying to include some testing and small refactors to help prevent regressions as I am about to start making some additions based on an earlier discussion I had this morning.

Copy link
Collaborator

@dhosterman dhosterman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to me, though I worry that things are going to be in a lot of flux and that these tests will become outdated very quickly. In particular, things like "66% safe responses" are very tenuous.

But if these are helpful to your comfort with refactoring, I'm all for it!

@mkly
Copy link
Member Author

mkly commented Feb 2, 2024

things like "66% safe responses" are very tenuous.

Indeed. It is more scaffolding that anything else. Those will get updated very soon to more meaningful values as I get a better understanding of what is going on with the code. I just needed to get something initially up and didn't want to waste a lot of time trying to be perfect. Thanks for taking a look.

@mkly mkly merged commit d3deae2 into main Feb 2, 2024
2 checks passed
@mkly mkly deleted the mkly/macro-tests-2 branch February 2, 2024 00:50
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants