Merge pull request #942 from godot-rust/bugfix/register-docs-tests

Bromeon · web-flow · commit 387bd8178841 · 2024-11-05T17:22:13.000Z
Fix `register-docs` feature not being tested
diff --git a/.github/workflows/full-ci.yml b/.github/workflows/full-ci.yml
@@ -13,7 +13,9 @@ on:
 
 
 env:
-  GDEXT_FEATURES: ''
+  # Applies to all 'register-docs' features across crates.
+  CLIPPY_FEATURES: '--features register-docs'
+  TEST_FEATURES: ''
   RETRY: ${{ github.workspace }}/.github/other/retry.sh
 
   # ASan options: https://github.com/google/sanitizers/wiki/AddressSanitizerFlags
@@ -99,7 +101,7 @@ jobs:
       # Note: could use `-- --no-deps` to not lint dependencies, however it doesn't really speed up and also skips deps in workspace.
       - name: "Check clippy"
         run: |
-          cargo clippy --all-targets $GDEXT_FEATURES -- \
+          cargo clippy --all-targets $CLIPPY_FEATURES -- \
           -D clippy::suspicious \
           -D clippy::style \
           -D clippy::complexity \
@@ -161,10 +163,10 @@ jobs:
         run: cargo +nightly update -Z minimal-versions
 
       - name: "Compile tests"
-        run: cargo test $GDEXT_FEATURES --no-run ${{ matrix.rust-extra-args }}
+        run: cargo test $TEST_FEATURES --no-run ${{ matrix.rust-extra-args }}
 
       - name: "Test"
-        run: cargo test $GDEXT_FEATURES ${{ matrix.rust-extra-args }}
+        run: cargo test $TEST_FEATURES ${{ matrix.rust-extra-args }}
 
 
   miri-test:
diff --git a/.github/workflows/minimal-ci.yml b/.github/workflows/minimal-ci.yml
@@ -21,8 +21,9 @@ on:
 
 
 env:
-  GDEXT_FEATURES: ''
-  #  GDEXT_FEATURES: '--features crate/feature'
+  # Applies to all 'register-docs' features across crates.
+  CLIPPY_FEATURES: '--features register-docs'
+  TEST_FEATURES: ''
   #  GDEXT_CRATE_ARGS: '-p godot-codegen -p godot-ffi -p godot-core -p godot-macros -p godot'
   RETRY: ${{ github.workspace }}/.github/other/retry.sh
 
@@ -96,7 +97,7 @@ jobs:
 
       - name: "Check clippy"
         run: |
-          cargo clippy --all-targets $GDEXT_FEATURES -- \
+          cargo clippy --all-targets $CLIPPY_FEATURES -- \
           -D clippy::suspicious \
           -D clippy::style \
           -D clippy::complexity \
@@ -120,10 +121,10 @@ jobs:
         uses: ./.github/composite/rust
 
       - name: "Compile tests"
-        run: cargo test $GDEXT_FEATURES --no-run
+        run: cargo test $TEST_FEATURES --no-run
 
       - name: "Test"
-        run: cargo test $GDEXT_FEATURES
+        run: cargo test $TEST_FEATURES
 
 
   # For complex matrix workflow, see https://stackoverflow.com/a/65434401
diff --git a/godot-core/Cargo.toml b/godot-core/Cargo.toml
@@ -12,7 +12,7 @@ homepage = "https://godot-rust.github.io"
 
 [features]
 default = []
-docs = []
+register-docs = []
 codegen-rustfmt = ["godot-ffi/codegen-rustfmt", "godot-codegen/codegen-rustfmt"]
 codegen-full = ["godot-codegen/codegen-full"]
 codegen-lazy-fptrs = [
diff --git a/godot-core/src/docs.rs b/godot-core/src/docs.rs
@@ -89,6 +89,7 @@ pub fn gather_xml_docs() -> impl Iterator<Item = String> {
             PluginItem::Struct {
                 docs: Some(docs), ..
             } => map.entry(class_name).or_default().definition = docs,
+
             _ => (),
         }
     });
diff --git a/godot-core/src/init/mod.rs b/godot-core/src/init/mod.rs
@@ -143,7 +143,7 @@ unsafe fn gdext_on_level_init(level: InitLevel) {
             unsafe { ensure_godot_features_compatible() };
         }
         InitLevel::Editor => {
-            #[cfg(all(since_api = "4.3", feature = "docs"))]
+            #[cfg(all(since_api = "4.3", feature = "register-docs"))]
             // SAFETY: Godot binding is initialized, and this is called from the main thread.
             unsafe {
                 crate::docs::register();
diff --git a/godot-core/src/lib.rs b/godot-core/src/lib.rs
@@ -15,11 +15,11 @@
 pub mod builder;
 pub mod builtin;
 pub mod classes;
-#[cfg(all(since_api = "4.3", feature = "docs"))]
+#[cfg(all(since_api = "4.3", feature = "register-docs"))]
 pub mod docs;
 #[doc(hidden)]
 pub mod possibly_docs {
-    #[cfg(all(since_api = "4.3", feature = "docs"))]
+    #[cfg(all(since_api = "4.3", feature = "register-docs"))]
     pub use crate::docs::*;
 }
 pub mod global;
@@ -35,7 +35,7 @@ pub use godot_ffi as sys;
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Validations (see also godot/lib.rs)
 
-#[cfg(all(feature = "docs", before_api = "4.3"))]
+#[cfg(all(feature = "register-docs", before_api = "4.3"))]
 compile_error!("Generating editor docs for Rust symbols requires at least Godot 4.3.");
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------
diff --git a/godot-core/src/registry/class.rs b/godot-core/src/registry/class.rs
@@ -243,7 +243,7 @@ fn fill_class_info(item: PluginItem, c: &mut ClassRegistrationInfo) {
             is_editor_plugin,
             is_internal,
             is_instantiable,
-            #[cfg(all(since_api = "4.3", feature = "docs"))]
+            #[cfg(all(since_api = "4.3", feature = "register-docs"))]
                 docs: _,
         } => {
             c.parent_class_name = Some(base_class_name);
@@ -296,7 +296,7 @@ fn fill_class_info(item: PluginItem, c: &mut ClassRegistrationInfo) {
         PluginItem::InherentImpl(InherentImpl {
             register_methods_constants_fn,
             register_rpcs_fn: _,
-            #[cfg(all(since_api = "4.3", feature = "docs"))]
+            #[cfg(all(since_api = "4.3", feature = "register-docs"))]
                 docs: _,
         }) => {
             c.register_methods_constants_fn = Some(register_methods_constants_fn);
@@ -315,7 +315,7 @@ fn fill_class_info(item: PluginItem, c: &mut ClassRegistrationInfo) {
             user_free_property_list_fn,
             user_property_can_revert_fn,
             user_property_get_revert_fn,
-            #[cfg(all(since_api = "4.3", feature = "docs"))]
+            #[cfg(all(since_api = "4.3", feature = "register-docs"))]
                 virtual_method_docs: _,
         } => {
             c.user_register_fn = user_register_fn;
diff --git a/godot-core/src/registry/plugin.rs b/godot-core/src/registry/plugin.rs
@@ -5,7 +5,7 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
 
-#[cfg(all(since_api = "4.3", feature = "docs"))]
+#[cfg(all(since_api = "4.3", feature = "register-docs"))]
 use crate::docs::*;
 use crate::init::InitLevel;
 use crate::meta::ClassName;
@@ -65,7 +65,7 @@ pub struct InherentImpl {
     ///
     /// This function is called in [`UserClass::__before_ready()`](crate::obj::UserClass::__before_ready) definitions generated by the `#[derive(GodotClass)]` macro.
     pub register_rpcs_fn: Option<ErasedRegisterRpcsFn>,
-    #[cfg(all(since_api = "4.3", feature = "docs"))]
+    #[cfg(all(since_api = "4.3", feature = "register-docs"))]
     pub docs: InherentImplDocs,
 }
 
@@ -121,7 +121,7 @@ pub enum PluginItem {
 
         /// Whether the class has a default constructor.
         is_instantiable: bool,
-        #[cfg(all(since_api = "4.3", feature = "docs"))]
+        #[cfg(all(since_api = "4.3", feature = "register-docs"))]
         docs: Option<StructDocs>,
     },
 
@@ -130,7 +130,7 @@ pub enum PluginItem {
 
     /// Collected from `#[godot_api] impl I... for MyClass`.
     ITraitImpl {
-        #[cfg(all(since_api = "4.3", feature = "docs"))]
+        #[cfg(all(since_api = "4.3", feature = "register-docs"))]
         /// Virtual method documentation.
         virtual_method_docs: &'static str,
         /// Callback to user-defined `register_class` function.
diff --git a/godot-macros/Cargo.toml b/godot-macros/Cargo.toml
@@ -12,7 +12,7 @@ homepage = "https://godot-rust.github.io"
 
 [features]
 api-custom = ["godot-bindings/api-custom"]
-docs = ["dep:markdown"]
+register-docs = ["dep:markdown"]
 codegen-full = ["godot/__codegen-full"]
 
 [lib]
diff --git a/godot-macros/src/class/data_models/field.rs b/godot-macros/src/class/data_models/field.rs
@@ -15,7 +15,7 @@ pub struct Field {
     pub var: Option<FieldVar>,
     pub export: Option<FieldExport>,
     pub is_onready: bool,
-    #[cfg(feature = "docs")]
+    #[cfg(feature = "register-docs")]
     pub attributes: Vec<venial::Attribute>,
 }
 
@@ -28,7 +28,7 @@ impl Field {
             var: None,
             export: None,
             is_onready: false,
-            #[cfg(feature = "docs")]
+            #[cfg(feature = "register-docs")]
             attributes: field.attributes.clone(),
         }
     }
diff --git a/godot-macros/src/class/data_models/inherent_impl.rs b/godot-macros/src/class/data_models/inherent_impl.rs
@@ -74,9 +74,9 @@ pub fn transform_inherent_impl(mut impl_block: venial::Impl) -> ParseResult<Toke
     let (funcs, signals) = process_godot_fns(&class_name, &mut impl_block)?;
     let consts = process_godot_constants(&mut impl_block)?;
 
-    #[cfg(all(feature = "docs", since_api = "4.3"))]
+    #[cfg(all(feature = "register-docs", since_api = "4.3"))]
     let docs = crate::docs::make_inherent_impl_docs(&funcs, &consts, &signals);
-    #[cfg(not(all(feature = "docs", since_api = "4.3")))]
+    #[cfg(not(all(feature = "register-docs", since_api = "4.3")))]
     let docs = quote! {};
 
     let signal_registrations = make_signal_registrations(signals, &class_name_obj);
diff --git a/godot-macros/src/class/data_models/interface_trait_impl.rs b/godot-macros/src/class/data_models/interface_trait_impl.rs
@@ -42,9 +42,9 @@ pub fn transform_trait_impl(original_impl: venial::Impl) -> ParseResult<TokenStr
     let mut virtual_method_names = vec![];
 
     let prv = quote! { ::godot::private };
-    #[cfg(all(feature = "docs", since_api = "4.3"))]
+    #[cfg(all(feature = "register-docs", since_api = "4.3"))]
     let docs = crate::docs::make_virtual_impl_docs(&original_impl.body_items);
-    #[cfg(not(all(feature = "docs", since_api = "4.3")))]
+    #[cfg(not(all(feature = "register-docs", since_api = "4.3")))]
     let docs = quote! {};
     for item in original_impl.body_items.iter() {
         let method = if let venial::ImplMember::AssocFunction(f) = item {
diff --git a/godot-macros/src/class/derive_godot_class.rs b/godot-macros/src/class/derive_godot_class.rs
@@ -46,13 +46,13 @@ pub fn derive_godot_class(item: venial::Item) -> ParseResult<TokenStream> {
 
     let is_internal = struct_cfg.is_internal;
     let base_ty = &struct_cfg.base_ty;
-    #[cfg(all(feature = "docs", since_api = "4.3"))]
+    #[cfg(all(feature = "register-docs", since_api = "4.3"))]
     let docs = crate::docs::make_definition_docs(
         base_ty.to_string(),
         &class.attributes,
         &fields.all_fields,
     );
-    #[cfg(not(all(feature = "docs", since_api = "4.3")))]
+    #[cfg(not(all(feature = "register-docs", since_api = "4.3")))]
     let docs = quote! {};
     let base_class = quote! { ::godot::classes::#base_ty };
     let base_class_name_obj = util::class_name_obj(&base_class);
diff --git a/godot-macros/src/docs.rs b/godot-macros/src/docs.rs
@@ -21,7 +21,7 @@ pub fn make_definition_docs(
         let base_escaped = xml_escape(base);
         let desc_escaped = xml_escape(make_docs_from_attributes(description)?);
         let members = members
-            .into_iter()
+            .iter()
             .filter(|x| x.var.is_some() | x.export.is_some())
             .filter_map(member)
             .collect::<String>();
@@ -112,7 +112,7 @@ fn siphon_docs_from_attributes(doc: &[Attribute]) -> impl Iterator<Item = String
             _ => None,
         })
         .flat_map(|doc| {
-            doc.into_iter().map(|x| {
+            doc.iter().map(|x| {
                 x.to_string()
                     .trim_start_matches('r')
                     .trim_start_matches('#')
@@ -126,7 +126,7 @@ fn siphon_docs_from_attributes(doc: &[Attribute]) -> impl Iterator<Item = String
 
 fn xml_escape(value: String) -> String {
     // Most strings have no special characters, so this check helps avoid unnecessary string copying
-    if !value.contains(&['&', '<', '>', '"', '\'']) {
+    if !value.contains(['&', '<', '>', '"', '\'']) {
         return value;
     }
 
diff --git a/godot-macros/src/docs/markdown_converter.rs b/godot-macros/src/docs/markdown_converter.rs
@@ -79,7 +79,7 @@ fn walk_node(node: &md::Node, definitions: &HashMap<&str, &str>) -> Option<Strin
 
         Html(html) => html.value.clone(),
 
-        _ => walk_nodes(&node.children()?, definitions, ""),
+        _ => walk_nodes(node.children()?, definitions, ""),
     };
 
     Some(bbcode)
diff --git a/godot-macros/src/lib.rs b/godot-macros/src/lib.rs
@@ -13,7 +13,7 @@
 mod bench;
 mod class;
 mod derive;
-#[cfg(all(feature = "docs", since_api = "4.3"))]
+#[cfg(all(feature = "register-docs", since_api = "4.3"))]
 mod docs;
 mod gdextension;
 mod itest;
diff --git a/godot/Cargo.toml b/godot/Cargo.toml
@@ -24,7 +24,7 @@ codegen-rustfmt = ["godot-core/codegen-rustfmt"]
 lazy-function-tables = ["godot-core/codegen-lazy-fptrs"]
 serde = ["godot-core/serde"]
 
-register-docs = ["godot-macros/docs", "godot-core/docs"]
+register-docs = ["godot-macros/register-docs", "godot-core/register-docs"]
 
 api-custom = ["godot-core/api-custom"]
 # [version-sync] [[
diff --git a/itest/rust/Cargo.toml b/itest/rust/Cargo.toml
@@ -14,7 +14,7 @@ default = ["codegen-full"]
 codegen-full = ["godot/__codegen-full"]
 codegen-full-experimental = ["codegen-full", "godot/experimental-godot-api"]
 experimental-threads = ["godot/experimental-threads"]
-register-docs = ["godot/register-docs"] # TODO remove as soon as constant_test.rs checks bitfields with #[constant] proc-macro
+register-docs = ["godot/register-docs"]
 serde = ["dep:serde", "dep:serde_json", "godot/serde"]
 
 # Do not add features here that are 1:1 forwarded to the `godot` crate, unless they are needed by itest itself.
diff --git a/itest/rust/src/register_tests/constant_test.rs b/itest/rust/src/register_tests/constant_test.rs
@@ -164,7 +164,7 @@ impl godot::obj::cap::ImplementsGodotApi for HasOtherConstants {
     }
 }
 
-// TODO once this is done via proc-macro, remove `register-docs` feature from itest, and update CI workflows.
+// TODO once this is done via proc-macro, see if `register-docs` is still used in register_docs_test.rs. Otherwise, remove feature from Cargo.toml.
 godot::sys::plugin_add!(
     __GODOT_PLUGIN_REGISTRY in ::godot::private;
     ::godot::private::ClassPlugin {
@@ -174,7 +174,7 @@ godot::sys::plugin_add!(
                 raw: ::godot::private::callbacks::register_user_methods_constants::<HasOtherConstants>,
             },
             register_rpcs_fn: None,
-            #[cfg(all(since_api = "4.3", feature = "register-docs"))]
+            #[cfg(feature = "register-docs")]
             docs: ::godot::docs::InherentImplDocs::default(),
         }),
         init_level: HasOtherConstants::INIT_LEVEL,
diff --git a/itest/rust/src/register_tests/mod.rs b/itest/rust/src/register_tests/mod.rs
@@ -12,6 +12,7 @@ mod func_test;
 mod gdscript_ffi_test;
 mod naming_tests;
 mod option_ffi_test;
+mod register_docs_test;
 #[cfg(feature = "codegen-full")]
 mod rpc_test;
 mod var_test;
diff --git a/itest/rust/src/register_tests/register_docs_test.rs b/itest/rust/src/register_tests/register_docs_test.rs
@@ -1,10 +1,13 @@
-#![cfg(feature = "register-docs")]
 /*
  * Copyright (c) godot-rust; Bromeon and contributors.
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
+
+#![cfg(feature = "register-docs")]
+
+use crate::framework::itest;
 use godot::prelude::*;
 
 /// *documented* ~ **documented** ~ [AABB] < [pr](https://github.com/godot-rust/gdext/pull/748)
@@ -187,15 +190,25 @@ impl FairlyDocumented {
     fn documented_signal(p: Vector3, w: f64, node: Gd<Node>);
 }
 
-#[test]
-fn correct() {
+#[itest]
+fn test_register_docs() {
+    let xml = find_class_docs("FairlyDocumented");
+
     // Uncomment if implementation changes and expected output file should be rewritten.
-    // std::fs::write(
-    //     "tests/test_data/docs.xml",
-    //     godot_core::docs::gather_xml_docs().next().unwrap(),
-    // );
-    assert_eq!(
-        include_str!("test_data/docs.xml"),
-        godot_core::docs::gather_xml_docs().next().unwrap()
-    );
+    // std::fs::write("../rust/src/register_tests/res/registered_docs.xml", &xml)
+    //     .expect("failed to write docs XML file");
+
+    assert_eq!(include_str!("res/registered_docs.xml"), xml);
+}
+
+fn find_class_docs(class_name: &str) -> String {
+    let mut count = 0;
+    for xml in godot::docs::gather_xml_docs() {
+        count += 1;
+        if xml.contains(class_name) {
+            return xml;
+        }
+    }
+
+    panic!("Registered docs for class {class_name} not found in {count} XML files");
 }
diff --git a/itest/rust/src/register_tests/res/registered_docs.xml b/itest/rust/src/register_tests/res/registered_docs.xml
@@ -25,7 +25,7 @@ these</description>
   </description>
 </method>
 
-<method name="virtual_documented">
+<method name="_virtual_documented">
   <return type="()" />
   <param index="0" name="node" type="Gd &lt; Node &gt;" />
   <description>

Original file line number	Diff line number	Diff line change
`@@ -89,6 +89,7 @@ pub fn gather_xml_docs() -> impl Iterator<Item = String> {`
`89`	`89`	`PluginItem::Struct {`
`90`	`90`	`docs: Some(docs), ..`
`91`	`91`	`} => map.entry(class_name).or_default().definition = docs,`
	`92`	`+`
`92`	`93`	`_ => (),`
`93`	`94`	`}`
`94`	`95`	`});`
Original file line number	Diff line number	Diff line change
`@@ -143,7 +143,7 @@ unsafe fn gdext_on_level_init(level: InitLevel) {`
`143`	`143`	`unsafe { ensure_godot_features_compatible() };`
`144`	`144`	`}`
`145`	`145`	`InitLevel::Editor => {`
`146`		`- #[cfg(all(since_api = "4.3", feature = "docs"))]`
	`146`	`+ #[cfg(all(since_api = "4.3", feature = "register-docs"))]`
`147`	`147`	`// SAFETY: Godot binding is initialized, and this is called from the main thread.`
`148`	`148`	`unsafe {`
`149`	`149`	`crate::docs::register();`
Original file line number	Diff line number	Diff line change
`@@ -15,7 +15,7 @@ pub struct Field {`
`15`	`15`	`pub var: Option<FieldVar>,`
`16`	`16`	`pub export: Option<FieldExport>,`
`17`	`17`	`pub is_onready: bool,`
`18`		`- #[cfg(feature = "docs")]`
	`18`	`+ #[cfg(feature = "register-docs")]`
`19`	`19`	`pub attributes: Vec<venial::Attribute>,`
`20`	`20`	`}`
`21`	`21`
`@@ -28,7 +28,7 @@ impl Field {`
`28`	`28`	`var: None,`
`29`	`29`	`export: None,`
`30`	`30`	`is_onready: false,`
`31`		`- #[cfg(feature = "docs")]`
	`31`	`+ #[cfg(feature = "register-docs")]`
`32`	`32`	`attributes: field.attributes.clone(),`
`33`	`33`	`}`
`34`	`34`	`}`