Validate forge command outputs in docs #2687

franciszekjob · 2024-11-19T12:38:11Z

Relates #2479
Closes #2684

Introduced changes

Automatically run snforge commands which are present in the docc; if the output of respective output is present in the docs too, it's compared with the actual output from executed command
Allow ignoring commands (so they will not be executed)

Checklist

This reverts commit 62bb35a.

…//github.com/foundry-rs/starknet-foundry into franciszekjob/2479-2-shell-commands-validation

…ciszekjob/2479-2-shell-commands-validation

…s://github.com/foundry-rs/starknet-foundry into franciszekjob/2479-2-shell-commands-validation

crates/docs/src/validation.rs

franciszekjob · 2024-11-21T12:27:58Z

crates/forge/tests/e2e/docs_snippets_validation.rs

+            let temp = resolve_temp_dir(&package_name, &listings_to_packages_mapping);
+            let output = test_runner(&temp).args(args).assert();
+
+            assert_stdout_contains(output, snippet_output);
+        }
    }

-    print_success_message(snippets.len(), "snforge");
+    print_success_message(snippets.len(), snippet_type.as_str());
+}
+
+fn resolve_temp_dir(
+    package_name: &str,
+    listings_to_packages_mapping: &HashMap<String, Vec<String>>,
+) -> TempDir {
+    if is_package_from_docs_listings(package_name, listings_to_packages_mapping) {
+        setup_package_from_docs_listings(package_name, listings_to_packages_mapping)
+    } else if ["addition", "fibonacci"].contains(&package_name) {
+        setup_hello_workspace()
+    } else {
+        setup_package(package_name)
+    }
 }


note: Currently we use hello_workpaces package (from crates/forge/tests/data) in one example in docs. In future we can simplify this logic and remove is_package_from_docs_listings, once we use only packages from docs listings. Ref: #2698

…ngs.sh`" This reverts commit 216c10b.

cptartur · 2024-11-21T17:28:03Z

crates/sncast/tests/docs_snippets/validation.rs

@@ -63,8 +46,8 @@ fn test_docs_snippets() {
        let exit_code = output.status.code().unwrap_or_default();
        let stderr = String::from_utf8_lossy(&output.stderr);

-        assert_valid_snippet(exit_code != 2, snippet, "sncast", &stderr);
+        assert_valid_snippet(exit_code != 2, snippet, &stderr);


Why not check if it's 0?

cptartur · 2024-11-21T17:29:40Z

scripts/verify_cairo_listings.sh

@@ -1,4 +1,4 @@
 #!/bin/bash
 set -e

-for d in ./docs/listings/*; do (cd "$d" && scarb test); done
+for d in ./docs/listings/*; do (cd "$d" && scarb check); done


How did it work before?

cptartur · 2024-11-21T17:31:43Z

crates/forge/tests/e2e/common/runner.rs

+    if package_path.contains("docs/listings") {
+        scarb_toml["dev-dependencies"]["snforge_std"]
+            .as_table_mut()
+            .and_then(|snforge_std| snforge_std.remove("workspace"));
+    }


Don't think we should be hardcoding some docs specific behaviors here. This could be a separate function that shares the logic with this one.

+ I think you wouldn't have to change the interface of current setup_package_with_file_patterns as well.

I see that you created one below, but I'd try to change this so the interface of tests we already had remains unchanged.

cptartur · 2024-11-21T17:38:35Z

crates/docs/src/validation.rs

+pub fn create_listings_to_packages_mapping() -> HashMap<String, Vec<String>> {
+    let docs_listings_path = "../../docs/listings";
+    let mut mapping = HashMap::new();
+
+    for listing in WalkDir::new(docs_listings_path)
+        .min_depth(1)
+        .max_depth(1)
+        .into_iter()
+        .filter_map(Result::ok)
+        .filter(|e| e.file_type().is_dir())
+    {
+        let listing_path = listing.path();
+        let crates_dir = listing_path.join("crates");
+
+        if crates_dir.is_dir() {
+            let packages = list_packages_in_directory(&crates_dir);
+            if let Some(listing_name) = listing_path.file_name().and_then(|x| x.to_str()) {
+                if !packages.is_empty() {
+                    mapping.insert(listing_name.to_string(), packages);
+                }
+            }
+        }
+    }
+
+    mapping
+}


Isn't this logic a bit too complicated just to get a mapping between a directory name and it's related crates?

ddoktorski · 2024-11-22T10:15:11Z

crates/docs/src/validation.rs

+            let command_match = caps.get(2)?;
            let match_start = caps.get(0)?.start();
+            let output = caps.get(3).map(|m| m.as_str().to_string());
+            let ignored = caps.get(1).map_or(false, |m| m.as_str().contains("ignore"));


nit: Could you reorder this to have .get(0), .get(1) etc?

crates/docs/src/validation.rs

ddoktorski · 2024-11-22T10:52:33Z

crates/docs/src/validation.rs

    );
 }
+
+pub fn create_listings_to_packages_mapping() -> HashMap<String, Vec<String>> {


nit:

Suggested change

pub fn create_listings_to_packages_mapping() -> HashMap<String, Vec<String>> {

pub fn create_listing_to_packages_map() -> HashMap<String, Vec<String>> {

Applicable in other places to rename listings_to_packages_mapping -> listing_to_packages_map

ddoktorski · 2024-11-22T10:58:35Z

crates/docs/src/validation.rs

+    mapping
+}
+
+fn list_packages_in_directory(dir_path: &Path) -> Vec<String> {


Maybe we could leverage scarb metadata command to get packages names?

ddoktorski · 2024-11-22T11:03:09Z

crates/forge/tests/e2e/common/runner.rs

+        .map(|(listing_name, _)| listing_name.clone())
+}
+
+pub(crate) fn setup_package_from_docs_listings(


nit:

Suggested change

pub(crate) fn setup_package_from_docs_listings(

pub(crate) fn setup_package_from_docs_listing(

ddoktorski · 2024-11-22T11:03:35Z

crates/forge/tests/e2e/common/runner.rs

+
+pub(crate) fn setup_package_from_docs_listings(
+    package_name: &str,
+    packages_mapping: &HashMap<String, Vec<String>>,


nit:

Suggested change

packages_mapping: &HashMap<String, Vec<String>>,

listing_to_packages_map: &HashMap<String, Vec<String>>,

Also in other places

ddoktorski · 2024-11-22T11:24:53Z

crates/docs/src/validation.rs

+    pub fn get_re(&self) -> Regex {
+        let escaped_command = regex::escape(self.as_str());
+        let pattern = format!(
+            r"(?ms)^(?:<!--\s*(.*?)\s*-->\n)?```shell\n\$ ({escaped_command} .+?)\n```(?:\s*<details>\n<summary>Output:<\/summary>\n\n```shell\n([\s\S]+?)\n```[\s]*<\/details>)?"


Could you add a short comment explanation of this regex?

Co-authored-by: ddoktorski <45050160+ddoktorski@users.noreply.github.com>

… into franciszekjob/2479-3-shell-commands-running-validation

franciszekjob and others added 30 commits November 8, 2024 17:38

Add expandable shell snippet outputs

3c4a076

Change shell snippets to expandable blocks

f8b0058

Make expandable snippet open by default

4e45711

Make tree commands expandable

23493ab

Restore prompt in shell snippets

62bb35a

Revert "Restore prompt in shell snippets"

3ef847f

This reverts commit 62bb35a.

Restore prompt in shell snippets

304e423

Restore prompt in other shell snippets

f10ac8e

Fix forge version check snippet

be9c9e5

Add JS script to remove prompt sign when copying code

a17d919

Add validation of sncast snippets

432e366

Merge branch 'franciszekjob/2479-expandable-shell-snippets' of https:…

2cc325c

…//github.com/foundry-rs/starknet-foundry into franciszekjob/2479-2-shell-commands-validation

Remove unnecessary else branch

f5f3e41

Fix left docs snippets

beb0855

Make tree snippet expandable in first steps section

18ffce2

Fix typo

1d7f284

Fix typo

48a3fcd

Fix typos

0438352

Merge branch 'master' into franciszekjob/2479-expandable-shell-snippets

ccfac3f

Improve sncast snippets validation test

dca617d

Merge branch 'franciszekjob/2479-expandable-shell-snippets' of https:…

ed6daa0

…//github.com/foundry-rs/starknet-foundry into franciszekjob/2479-2-shell-commands-validation

Fix printing number of validated snippets in sncast test

3ae3107

Add snforge docs snippets validation test

2f65010

Remove print

efc2e1d

Merge branch 'master' into franciszekjob/2479-expandable-shell-snippets

2823523

Merge branch 'franciszekjob/2479-expandable-shell-snippets' into fran…

bcbb861

…ciszekjob/2479-2-shell-commands-validation

Move docs validation into separate crate

1b968be

Merge branch 'franciszekjob/2479-2-shell-commands-validation' of http…

041f604

…s://github.com/foundry-rs/starknet-foundry into franciszekjob/2479-2-shell-commands-validation

Fix newline

7bbcd99

Update Cargo.toml in sncast

bdcd467

Add todo

97df99f

franciszekjob marked this pull request as ready for review November 21, 2024 11:09

franciszekjob requested review from cptartur, ddoktorski and kkawula as code owners November 21, 2024 11:09

franciszekjob added 2 commits November 21, 2024 12:38

Remove unused package

dc4386c

Allow ignoring snippets

4d245af

Draggu reviewed Nov 21, 2024

View reviewed changes

crates/docs/src/validation.rs Outdated Show resolved Hide resolved

crates/docs/src/validation.rs Outdated Show resolved Hide resolved

Fix failing tests

0eadab4

franciszekjob commented Nov 21, 2024

View reviewed changes

franciszekjob added 4 commits November 21, 2024 13:39

Apply CR suggestions

c5e3b19

Add hello_workspaces case in resolve_temp_dir

4df85ac

Run scarb clean before scarb check in verify_cairo_listings.sh

216c10b

Revert "Run scarb clean before scarb check in `verify_cairo_listi…

a2f01d5

…ngs.sh`" This reverts commit 216c10b.

cptartur reviewed Nov 21, 2024

View reviewed changes

franciszekjob added 3 commits November 21, 2024 19:20

Reorganize listings packages

a00a460

Update manifests in listings packages

ad9aa26

Add hello_workspace package

12edd48

ddoktorski reviewed Nov 22, 2024

View reviewed changes

franciszekjob and others added 11 commits November 22, 2024 15:52

Add SnippetConfig

fd78dd4

Refactor setup_package_with_file_patterns

e2eb456

Fix typo in packages manifests

3dd6452

Add config to snippets in markdown files

58bac96

Refactor extract_snippets_from_file

a951882

Remove unneeded changes while using setup_package_with_file_patterns

538a775

Update crates/docs/src/validation.rs

2a7ea51

Co-authored-by: ddoktorski <45050160+ddoktorski@users.noreply.github.com>

Trigger CI

4d18e0a

Merge branch 'master' of https://github.com/foundry-rs/starknet-foundry…

f52f64e

… into franciszekjob/2479-3-shell-commands-running-validation

Update manifests

e94a85b

Update listings embeds

393cc29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate forge command outputs in docs #2687

Validate forge command outputs in docs #2687

franciszekjob commented Nov 19, 2024 •

edited

Loading

franciszekjob Nov 21, 2024

cptartur Nov 21, 2024

cptartur Nov 21, 2024

cptartur Nov 21, 2024

cptartur Nov 21, 2024

cptartur Nov 21, 2024

ddoktorski Nov 22, 2024

ddoktorski Nov 22, 2024

ddoktorski Nov 22, 2024

ddoktorski Nov 22, 2024

ddoktorski Nov 22, 2024

ddoktorski Nov 22, 2024

	pub fn create_listings_to_packages_mapping() -> HashMap<String, Vec<String>> {
	pub fn create_listing_to_packages_map() -> HashMap<String, Vec<String>> {

	pub(crate) fn setup_package_from_docs_listings(
	pub(crate) fn setup_package_from_docs_listing(

	packages_mapping: &HashMap<String, Vec<String>>,
	listing_to_packages_map: &HashMap<String, Vec<String>>,

Validate forge command outputs in docs #2687

Are you sure you want to change the base?

Validate forge command outputs in docs #2687

Conversation

franciszekjob commented Nov 19, 2024 • edited Loading

Introduced changes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franciszekjob commented Nov 19, 2024 •

edited

Loading