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

Full code test coverage #31

Merged
merged 12 commits into from
Sep 21, 2021
Merged

Full code test coverage #31

merged 12 commits into from
Sep 21, 2021

Conversation

weswalla
Copy link
Collaborator

@weswalla weswalla commented Sep 14, 2021

Currently have test coverage of public zome functions in the projects zome.
I left out test functions for whoami and fetch_agents for now as they call hdk_crud functions and I'm not sure if we want to test those functions here. I see that you plan on doing test coverage over there eventually: lightningrodlabs/hdk_crud#1

Copy link
Collaborator

@Connoropolous Connoropolous left a comment

Choose a reason for hiding this comment

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

Great stuff... you are right about not testing the ones that use hdk_crud... better not to for now.

Let's go ahead and take that next step to create a fixturator for Profile.

You can see setting up a constructor for a struct isn't too hard, see here:
https://github.com/h-be/acorn/blob/c473a11155b3681d666ef42e57328ffe46252ca7/dna/zomes/projects/src/project/edge/crud.rs#L19-L33

Then make the series of types in the fixturator that you started match the list of types that the constructor takes.

@@ -0,0 +1,57 @@
use hdk::prelude::*;

// should these be inside a module in this file?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// should these be inside a module in this file?

Comment on lines +74 to +79
let payload = ExternIO::encode(signal).unwrap();
let agents = vec![];
let remote_signal = RemoteSignal {
signal: ExternIO::encode(payload).unwrap(),
agents,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh so you got this part, well done

@weswalla
Copy link
Collaborator Author

I think this is a good place to leave the test coverage for now. All the other public zome functions call hdk_crud functions. I don't think it makes sense testing those functions over here, what do you think?

@weswalla weswalla marked this pull request as ready for review September 15, 2021 23:47
Copy link
Collaborator

@Connoropolous Connoropolous left a comment

Choose a reason for hiding this comment

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

this is great, very good work!

my comment is mostly a follow up thing that we can do... I'd say this could be merged.

It'd be nice to have the files be formatted, there's an easy way to do that, over a whole project folder actually. It could be done as a follow up.
cargo fmt within the folder of the crate.
It should be done as its own commit. Anyways, that's just another follow up.
This can get merged!

Next actions for you might be to capture these follow ups as issues, here, and in hdk_crud

dna/tests/src/fixtures.rs Show resolved Hide resolved
Comment on lines +19 to +27
fixturator!(
WrappedHeaderHash;
constructor fn new(HeaderHash);
);

fixturator!(
WrappedAgentPubKey;
constructor fn new(AgentPubKey);
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this comment is not for now, but for later...

these two themselves should go over into hdk_crud, since that is where these types are defined.

Also, tell me what you think of this... renaming WrappedHeaderHash to WrappedHeaderAddress and start making consistent at least for our stuff that terminology

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm, the hdk uses HeaderHash and that is the type that is being wrapped changing it to address could be confusing for some? Plus Hash uses a few less characters. Although I may be missing some important context as to why Address makes more sense

Copy link
Collaborator

@Connoropolous Connoropolous Sep 16, 2021

Choose a reason for hiding this comment

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

Ah so you feel Hash could be the term to focus and land on. Fair. We will never be able to escape places in function names in the HDK where it says address but alas thats ok

@Connoropolous Connoropolous merged commit a4fb845 into main Sep 21, 2021
@Connoropolous Connoropolous deleted the full-test-coverage branch July 6, 2022 18:12
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