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

Add hardhat task to update programs. #49

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

itirabasso
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@plaintextpaco plaintextpaco left a comment

Choose a reason for hiding this comment

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

  describe("WHEN adding it with the add-program task, AND passing all parameters", () => {
    it("THEN it is listed", async () => {
    it("WHEN updates the program using update-program", () => {
      it("WHEN updates name and url", async () => {
      it("WHEN updates only name", async () => {
      it("WHEN updates only url", async () => {

should be:

  describe("WHEN adding it with the add-program task, AND passing all parameters", () => {
    it("THEN it is listed", async () => {
    describe("WHEN updating both name and url with the update-program task", () => {
      it("THEN the name changed")
      it("AND the url changed")
    describe("WHEN updating only the name with the update-program task")
      it("THEN the name changed")
      it("AND the url is kept")
    describe("WHEN updating only the url with the update-program task")
      it("THEN the url changed")
      it("AND the name is kept")

or at least:

  describe("WHEN adding it with the add-program task, AND passing all parameters", () => {
    it("THEN it is listed", async () => {
    describe("update-program task", () => {
      it("WHEN updating name and url THEN they are both modified", async () => {
      it("WHEN updating only name THEN only the name changes AND the original url is kept", async () => {
      it("WHEN updating only url THEN only the url changes AND the original name is kept", async () => {

also, there's the missing testcase of not passing any of the optional flags

hardhat.config.ts Show resolved Hide resolved
tasks/aludel.ts Show resolved Hide resolved
test/deployments.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@plaintextpaco plaintextpaco left a comment

Choose a reason for hiding this comment

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

The CI fails for something other than the Alchemy 429, so please fix that as well.

the other blocking change is the incorrect nesting of mocha hooks

.env.example Outdated Show resolved Hide resolved
tasks/aludel.ts Outdated Show resolved Hide resolved
test/deployments.ts Outdated Show resolved Hide resolved
test/deployments.ts Show resolved Hide resolved
test/deployments.ts Show resolved Hide resolved
@itirabasso itirabasso force-pushed the update-program branch 3 times, most recently from 8096873 to ae68b58 Compare January 20, 2023 18:23
Copy link
Collaborator

@plaintextpaco plaintextpaco left a comment

Choose a reason for hiding this comment

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

fix those minor changes, rebase, and we're good to go

tasks/aludel.ts Outdated Show resolved Hide resolved
expect(updatedProgram.startTime).to.eq(program.startTime);
});
});
describe("AND updates url", async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

fix the grammar of the descriptions.

this should perhaps be AND WHEN updating only the url

The test logs are harder to follow otherwise

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wouldn't WHEN be redundant?

          WHEN updates the program using update-program
            AND updates the name
              ✓ THEN only the name is changed

Copy link
Collaborator

Choose a reason for hiding this comment

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

have you taken the previous feedback into account?

use proper WHEN-THEN syntaxis

update tests

grammar
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.

2 participants