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

importing test transactions #773

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yuunlimm
Copy link
Contributor

@yuunlimm yuunlimm commented Jan 17, 2025

Description

Screenshot 2025-01-17 at 9.45.03 AM.png

Checklist

  • If any existing pages were renamed or removed:
    • Were redirects added to next.config.mjs?
    • Did you update any relative links that pointed to the renamed / removed pages?
  • Do all Lints pass?
    • [x ] Have you ran pnpm fmt?
    • [x ] Have you ran pnpm lint?

Copy link

vercel bot commented Jan 17, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
developer-docs-nextra ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 23, 2025 9:44pm

Copy link
Contributor Author

yuunlimm commented Jan 17, 2025

@yuunlimm yuunlimm marked this pull request as ready for review January 17, 2025 17:45
@yuunlimm yuunlimm requested review from a team, gregnazario, hariria and igor-p as code owners January 17, 2025 17:45
@yuunlimm yuunlimm force-pushed the 01-17-importing_test_transactions branch 4 times, most recently from 51b360e to 0307421 Compare January 23, 2025 18:30
@yuunlimm yuunlimm force-pushed the 01-17-importing_test_transactions branch from 0307421 to d1e31ef Compare January 23, 2025 18:33

### Export the `json_transactions` Folder

Ensure the `json_transactions` folder is properly exported in the library file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Bit confused about this instruction and the "Export the Generated File" instruction. Do they need to add both to mod.rs?

pub mod generated_transactions;
pub mod json_transactions;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

first one they need for sure, but the 2nd one could be slightly different. if they already have their own file where they are exporting othe folders they can just add to there. should I just remove that part? I assume they will know how to export files..

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh is this for generated_transactions to be able to read json_transactions?

Then it makes sense to include pub mod json_transactions in the instructions, but we can add more details to explain why you need to do it and which folder to do it in.

The previous step and this step refer to different repos, so it's a bit confusing to follow.

Copy link
Contributor Author

@yuunlimm yuunlimm Jan 23, 2025

Choose a reason for hiding this comment

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

yeah I will add more details and update the url to use the same repo as teh step above!

Comment on lines 102 to 104
### Add as a Dependency

Include the crate containing the generated transactions as a dependency in the `Cargo.toml` file of your test crate. (Internally, we store transactions in `aptos-core` and use them in the [processor repo](https://github.com/aptos-labs/aptos-indexer-processors/blob/0c92d323b0f560b5f8601a831a36520ad9b72d68/rust/Cargo.toml#L34)).
Copy link
Contributor

Choose a reason for hiding this comment

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

Users have the choice of --output-folder, they could put it in a new crate or existing crate, so I think we can skip this step in this guide.

[Example Crate](https://github.com/aptos-labs/aptos-indexer-processor-example/tree/main/test-transactions-example).

## Next Steps
Once the transaction constants are integrated, you can use them in processor tests to validate functionality. For detailed instructions on writing processor tests, refer to [Writing Processor Tests]().
Copy link

Choose a reason for hiding this comment

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

The link [Writing Processor Tests]() appears to be a placeholder with an empty destination URL. To maintain documentation quality, either add the correct URL for the processor tests documentation or remove the link formatting entirely. This will prevent users from encountering a broken link when following the guide.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

### Integrate into Test Cases

Use the exported transaction constants directly in your test cases to simulate real transactions and validate processing logic.

Copy link

Choose a reason for hiding this comment

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

Remove or update the broken link 'Writing Processor Tests' at the end of the file. If the documentation doesn't exist yet, remove the reference. If it does exist, update with the correct path.

Spotted by Graphite Reviewer (based on CI logs)

Is this helpful? React 👍 or 👎 to let us know.

Comment on lines +21 to +23
"advanced-tutorials": {
title: "Advanced Tutorials",
},
Copy link
Contributor

Choose a reason for hiding this comment

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

there's already an Advanced section. can remove this.

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