-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add a python package with doit workflows and logic for building and testing jupyter products #90
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments - many of which are questions for my own understanding and to better scope what next steps are needed.
Overall:
- @tonyfast this was a big piece of work 🤝 well done on getting all these moving pieces together
- we could merge this and treat as our first step towards a maintainer tool
- the only changes I would "request" is renaming some methods and classes to give more meaningful names as I know I am going to struggle later on
yield from (self.path / where).rglob("*/package.json") | ||
|
||
# cloning requires the env | ||
# everything else requires cloning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am somehow confused by this statement but still need to go through some other bits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the next step in this work is to set up the conditions for completed tasks in do it. we'll need to build out the DAG.
if someone does not have git
then they will need a conda
environment ahead of time. these are notes for me to explicitly express the logic for running the tasks.
name="yarn", | ||
actions=[do(f"{self.conda} yarn install", cwd=target)], | ||
) | ||
if CI or GITPOD: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how playwright caching works? maybe @gabalafou does - but for now this will be our starting point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea.... this is going to take a meeting of the minds. there should be a way to inspect the state of their cache. might take a little pair programming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check out caching browsers in the Playwright docs
As for the action on PR - we did not get to this on Monday but I have now a good mental picture of what is needed + some ideas on how we can expose the reports. Will add these to the issues to plan for the next iteration 🎉 thanks for seeing this through @tonyfast - this will be a great foundation to build from |
Co-authored-by: Tania Allard <[email protected]>
i think i addressed as much as i could here. hope y'all have a time for another review. |
Summary of changes my previous commit:
Things I want to do in a future commit:
Maybe in this PR or a future PR, I also want to remove all of the duplicated code under I don't want the testing code to live in two places, so I guess that in order to both have a single source of truth for testing code and to keep the Gitpod work, I'll need to follow this up with code that ports the Gitpod config to work with the code in this branch, then I'll be able to delete everything under |
}, | ||
"dependencies": { | ||
"@jupyterlab/galata": "4.0.2", | ||
"expect-axe-playwright": "gabalafou/expect-axe-playwright#f8af64b" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has to be fixed.
I forked expect-axe-playwright because its API wasn't flexible enough for what we want to do.
I see two possible paths forward:
- Keep dependency. Upstream my changes to expect-axe-playwright (will probably end up taking on a new form)
- Discard dependency. Copy-paste whatever code is useful from expect-axe-playwright into our own source code in this repo and thereby get rid of the dependency altogether.
I'm not sure which approach is best. I lean slightly towards the second option simply because these additional libraries like expect-axe-playwright are very slim. I'm not sure they bring enough to the table to outweigh the downsides to being dependent on them. The only dependency where I don't want to reinvent the wheel is axe-reporter-html
, which provides a nice HTML template for summarizing the axe-core violations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the changes you made in your fork are to export the output files?
other options:
- could we include our patched version of axe fixture in this library itself, and ship it as part of the a package?
- is this something we'd really want to target in
galata
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was reviewing PRs and noticed this is hanging. Is there anything I can do to help move the discussion forward?
Woohoo! I'm merging this since it has approval and feedback has been addressed. Thank y'all for your work on this. |
This pull request adds some tooling and structure for provisioning, building, and testing jupyter products. the features it adds are:
gitpod
pull requests with a minor intervention.the things it lacks:
@gabalafou i hope having the branch over here helps with editing and review.
here goes nothing. lets see if the ci goes 🟢 here 🤞
Fixes: Quansight-Labs/jupyter-a11y-mgmt#102
Fixes: Quansight-Labs/jupyter-a11y-mgmt#91