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

Get tests passing on main #16

Closed
wants to merge 2 commits into from

Conversation

emwalker
Copy link
Collaborator

@emwalker emwalker commented Sep 23, 2023

There's several things you'd ideally have for a Rust project that people are collaborating on:

  1. cargo test passes on main
  2. cargo build --release works on main
  3. No changes occur when cargo fmt is run
  4. No changes occur or warnings are issued when cargo clippy --fix is run
  5. There's a rust-toolchain.toml file that specifies the version of Rust being used
  6. No warnings are printed to the console when tests or a build are run

In this PR, we take care of (1) and (2), above. After this PR, you'd want to keep any work that breaks (1) or (2) on a branch and fix things before merging back into main.

Let me know if this and similar PRs are helpful.

2023-09-23_15-49

2023-09-23_15-51

@emwalker emwalker marked this pull request as draft September 23, 2023 21:54
@emwalker
Copy link
Collaborator Author

Converting to draft to fix those conflicts.

@emwalker emwalker marked this pull request as ready for review September 23, 2023 22:04
"#
);
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These tests would be reintroduced once they're all passing on a topic branch.

Copy link
Member

Choose a reason for hiding this comment

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

i agree.. I'm currently trying to fix those tests, but we can remove them for now. My goal is indeed a stable/compilable main branch

@emwalker emwalker mentioned this pull request Sep 23, 2023
@jaytaph
Copy link
Member

jaytaph commented Sep 24, 2023

I've updated the merge conflicts (some tests are already fixed). Also there is since yesterday some additional CI tests like fmt and clippy running, so that should take care of point 3 and 4.

I'm not familiar with rust-toolchain.toml, but it sounds a good thing :)

@jaytaph
Copy link
Member

jaytaph commented Sep 24, 2023

@emwalker : More conflicts arise as I'm also am fixing the tests :) Right now, we have a CI passing main branch with only the document parsing test set to #[ignore]. As making sure that parsing works properly is currently my main focus, i'm happy to keep the test as ignore in the code-base for now.

I think it would be better to close this PR for now, but i really like the steps you have layed out on getting a good repo up and running. Would you be able to help with the rust-toolchain.toml, and maybe verify if I did everything correct with the other steps? That would really be appreciated!

@jaytaph jaytaph closed this Sep 24, 2023
@emwalker
Copy link
Collaborator Author

I think it would be better to close this PR for now, but i really like the steps you have layed out on getting a good repo up and running. Would you be able to help with the rust-toolchain.toml, and maybe verify if I did everything correct with the other steps? That would really be appreciated!

@jaytaph sure, sounds good. I'll take a look.

@emwalker
Copy link
Collaborator Author

@jaytaph here's what I'm seeing:

  1. cargo test passes ✅
  2. cargo build --release completes as expected ✅
  3. cargo fmt results in no changes ✅
  4. cargo clippy --fix results in changes ❌
  5. no compilation time warnings ✅

I'll create a PR with a rust-toolchain.toml file. Let me know if you want me to pick up (4) as well.

@emwalker emwalker deleted the clean-up-main-tests branch September 24, 2023 13:26
@jaytaph
Copy link
Member

jaytaph commented Sep 24, 2023

@emwalker yes. clippy fix should pass as well. I think i've fixed those in a branch i'm working on, but if you can create a separate PR for that, that's would be ok.

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