From 723f73a0c667b9c1d6f62f92ba1362cc1c2133a8 Mon Sep 17 00:00:00 2001
From: lindsay stevens <lstevens@getodk.org>
Date: Wed, 29 Nov 2023 21:38:18 +1100
Subject: [PATCH] fix: preserve order of columns when building secondary
 instance

- Seems like main reason for sorting in the first place was to help
  satisfy string-based matching for XML output tests. Since there are
  XPath test facilities now, it's not necessary.
- fix: remove explicit sorting in survey.py and utils.py
- fix: remove order randomisation in xls2json.py via conversion of
  keys to set objects - use dicts instead so that order is preserved.
- fix: tests broken by the above changes
- fix: add retry loop for shutting down server between test runs.
- add: test asserting column order for additional choices columns
---
 pyxform/survey.py                |  2 +-
 pyxform/utils.py                 |  6 ++----
 pyxform/xls2json.py              | 11 +++++++----
 tests/fixtures/strings.ini       |  2 --
 tests/j2x_question_tests.py      | 33 +++++++++++++++++++++-----------
 tests/test_choices_sheet.py      | 29 ++++++++++++++++++++++++++++
 tests/test_external_instances.py | 18 ++++-------------
 tests/test_whitespace.py         | 19 ++++++++++++------
 tests/validators/server.py       | 12 +++++++++---
 9 files changed, 87 insertions(+), 45 deletions(-)

diff --git a/pyxform/survey.py b/pyxform/survey.py
index 242fbcebd..e0061c173 100644
--- a/pyxform/survey.py
+++ b/pyxform/survey.py
@@ -296,7 +296,7 @@ def _generate_static_instances(list_name, choice_list) -> InstanceInfo:
                 itext_id = "-".join([list_name, str(idx)])
                 choice_element_list.append(node("itextId", itext_id))
 
-            for name, value in sorted(choice.items()):
+            for name, value in choice.items():
                 if isinstance(value, str) and name != "label":
                     choice_element_list.append(node(name, str(value)))
                 if (
diff --git a/pyxform/utils.py b/pyxform/utils.py
index 7786cb1cb..a5d8c926b 100644
--- a/pyxform/utils.py
+++ b/pyxform/utils.py
@@ -80,10 +80,8 @@ def node(*args, **kwargs) -> DetachableElement:
     assert len(unicode_args) <= 1
     parsed_string = False
 
-    # Convert the kwargs xml attribute dictionary to a xml.dom.minidom.Element. Sort the
-    # attributes to guarantee a consistent order across Python versions.
-    # See pyxform_test_case.reorder_attributes for details.
-    for k, v in iter(sorted(kwargs.items())):
+    # Convert the kwargs xml attribute dictionary to a xml.dom.minidom.Element.
+    for k, v in iter(kwargs.items()):
         if k in blocked_attributes:
             continue
         if k == "toParseString":
diff --git a/pyxform/xls2json.py b/pyxform/xls2json.py
index d21f3644e..21add16f5 100644
--- a/pyxform/xls2json.py
+++ b/pyxform/xls2json.py
@@ -58,19 +58,22 @@ def merge_dicts(dict_a, dict_b, default_key="default"):
     if dict_b is None or dict_b == {}:
         return dict_a
 
-    if type(dict_a) is not dict:
+    if not isinstance(dict_a, dict):
         if default_key in dict_b:
             return dict_b
         dict_a = {default_key: dict_a}
-    if type(dict_b) is not dict:
+    if not isinstance(dict_b, dict):
         if default_key in dict_a:
             return dict_a
         dict_b = {default_key: dict_b}
 
-    all_keys = set(dict_a.keys()).union(set(dict_b.keys()))
+    # Union keys but retain order (as opposed to set()), preferencing dict_a then dict_b.
+    # E.g. {"a": 1, "b": 2} + {"c": 3, "a": 4} -> {"a": None, "b": None, "c": None}
+    all_keys = {k: None for k in dict_a.keys()}
+    all_keys.update({k: None for k in dict_b.keys()})
 
     out_dict = dict()
-    for key in all_keys:
+    for key in all_keys.keys():
         out_dict[key] = merge_dicts(dict_a.get(key), dict_b.get(key), default_key)
     return out_dict
 
diff --git a/tests/fixtures/strings.ini b/tests/fixtures/strings.ini
index a46e9cded..90dfedd34 100644
--- a/tests/fixtures/strings.ini
+++ b/tests/fixtures/strings.ini
@@ -11,8 +11,6 @@ test_simple_integer_question_type_multilingual_control = <input ref="/test/integ
 test_simple_integer_question_type_multilingual_binding = <bind nodeset="/test/integer_q" type="int"/>
 test_simple_date_question_type_multilingual_control = <input ref="/test/date_q"><label ref="jr:itext('/test/date_q:label')"/></input>
 test_simple_date_question_type_multilingual_binding = <bind nodeset="/test/date_q" type="date"/>
-test_simple_phone_number_question_type_multilingual_control = <input ref="/test/phone_number_q"><label ref="jr:itext('/test/phone_number_q:label')"/><hint>Enter numbers only.</hint></input>
-test_simple_phone_number_question_type_multilingual_binding = <bind constraint="regex(., '^\d*$')" nodeset="/test/phone_number_q" type="string"/>
 test_simple_select_all_question_multilingual_control = <select ref="/test/select_all_q"><label ref="jr:itext('/test/select_all_q:label')"/><item><label ref="jr:itext('/test/select_all_q/f:label')"/><value>f</value></item><item><label ref="jr:itext('/test/select_all_q/g:label')"/><value>g</value></item><item><label ref="jr:itext('/test/select_all_q/h:label')"/><value>h</value></item></select>
 test_simple_select_all_question_multilingual_binding = <bind nodeset="/test/select_all_q" type="string"/>
 test_simple_decimal_question_multilingual_control = <input ref="/test/decimal_q"><label ref="jr:itext('/test/decimal_q:label')"/></input>
diff --git a/tests/j2x_question_tests.py b/tests/j2x_question_tests.py
index bc242adef..f04657ba2 100644
--- a/tests/j2x_question_tests.py
+++ b/tests/j2x_question_tests.py
@@ -154,20 +154,31 @@ def test_simple_phone_number_question_type_multilingual(self):
             "name": "phone_number_q",
         }
 
-        expected_phone_number_control_xml = self.config.get(
-            self.cls_name, "test_simple_phone_number_question_type_multilingual_control"
-        )
-
-        expected_phone_number_binding_xml = self.config.get(
-            self.cls_name, "test_simple_phone_number_question_type_multilingual_binding"
-        )
-
         q = create_survey_element_from_dict(simple_phone_number_question)
         self.s.add_child(q)
-        self.assertEqual(ctw(q.xml_control()), expected_phone_number_control_xml)
 
-        if TESTING_BINDINGS:
-            self.assertEqual(ctw(q.xml_bindings()), expected_phone_number_binding_xml)
+        # Inspect XML Control
+        observed = q.xml_control()
+        self.assertEqual("input", observed.nodeName)
+        self.assertEqual("/test/phone_number_q", observed.attributes["ref"].nodeValue)
+        observed_label = observed.childNodes[0]
+        self.assertEqual("label", observed_label.nodeName)
+        self.assertEqual(
+            "jr:itext('/test/phone_number_q:label')",
+            observed_label.attributes["ref"].nodeValue,
+        )
+        observed_hint = observed.childNodes[1]
+        self.assertEqual("hint", observed_hint.nodeName)
+        self.assertEqual("Enter numbers only.", observed_hint.childNodes[0].nodeValue)
+
+        # Inspect XML Binding
+        expected = {
+            "nodeset": "/test/phone_number_q",
+            "type": "string",
+            "constraint": r"regex(., '^\d*$')",
+        }
+        observed = {k: v for k, v in q.xml_bindings()[0].attributes.items()}
+        self.assertDictEqual(expected, observed)
 
     def test_simple_select_all_question_multilingual(self):
         """
diff --git a/tests/test_choices_sheet.py b/tests/test_choices_sheet.py
index 9539f2567..ffecf3c20 100644
--- a/tests/test_choices_sheet.py
+++ b/tests/test_choices_sheet.py
@@ -106,3 +106,32 @@ def test_choices_without_labels__for_dynamic_selects__allowed_by_pyxform(self):
                 """,
             ],
         )
+
+    def test_choices_extra_columns_output_order_matches_xlsform(self):
+        """Should find that element order matches column order."""
+        md = """
+        | survey   |                    |      |       |
+        |          | type               | name | label |
+        |          | select_one choices | a    | A     |
+        | choices  |                    |      |       |
+        |          | list_name          | name | label | geometry                 |
+        |          | choices            | 1    |       | 46.5841618 7.0801379 0 0 |
+        |          | choices            | 2    |       | 35.8805082 76.515057 0 0 |
+        """
+        self.assertPyxformXform(
+            md=md,
+            xml__xpath_contains=[
+                """
+                /h:html/h:head/x:model/x:instance[@id='choices']/x:root/x:item[
+                  ./x:name[position() = 1 and text() = '1']
+                  and ./x:geometry[position() = 2]
+                ]
+                """
+                """
+                /h:html/h:head/x:model/x:instance[@id='choices']/x:root/x:item[
+                  ./x:name[position() = 1 and text() = '2']
+                  and ./x:geometry[position() = 2]
+                ]
+                """
+            ],
+        )
diff --git a/tests/test_external_instances.py b/tests/test_external_instances.py
index c9775b460..386c54846 100644
--- a/tests/test_external_instances.py
+++ b/tests/test_external_instances.py
@@ -8,6 +8,7 @@
 
 from pyxform.errors import PyXFormError
 from tests.pyxform_test_case import PyxformTestCase, PyxformTestError
+from tests.xpath_helpers.choices import xpc
 
 
 class ExternalInstanceTests(PyxformTestCase):
@@ -229,21 +230,10 @@ def test_can__use_all_types_together_with_unique_ids(self):
                 '<instance id="city1" src="jr://file/city1.xml"/>',
                 '<instance id="cities" src="jr://file-csv/cities.csv"/>',
                 '<instance id="fruits" src="jr://file-csv/fruits.csv"/>',
-                """
-      <instance id="states">
-        <root>
-          <item>
-            <label>Pass</label>
-            <name>1</name>
-          </item>
-          <item>
-            <label>Fail</label>
-            <name>2</name>
-          </item>
-        </root>
-      </instance>
-""",
             ],  # noqa
+            xml__xpath_match=[
+                xpc.model_instance_choices_label("states", (("1", "Pass"), ("2", "Fail")))
+            ],
         )
 
     def test_cannot__use_different_src_same_id__select_then_internal(self):
diff --git a/tests/test_whitespace.py b/tests/test_whitespace.py
index 3b1156ad4..e74426e51 100644
--- a/tests/test_whitespace.py
+++ b/tests/test_whitespace.py
@@ -27,9 +27,16 @@ def test_values_without_whitespaces_are_processed_successfully(self):
             |                | id_string          | public_key | submission_url                                    |
             |                | tutorial_encrypted | MIIB       | https://odk.ona.io/random_person/submission       |
           """
-
-        survey = self.md_to_pyxform_survey(md_raw=md)
-        expected = """<submission action="https://odk.ona.io/random_person/submission" base64RsaPublicKey="MIIB" method="post"/>"""
-        xml = survey._to_pretty_xml()
-        self.assertEqual(1, xml.count(expected))
-        self.assertPyxformXform(md=md, xml__contains=expected, run_odk_validate=True)
+        self.assertPyxformXform(
+            md=md,
+            run_odk_validate=True,
+            xml__xpath_contains=[
+                """
+                /h:html/h:head/x:model/x:submission[
+                  @action='https://odk.ona.io/random_person/submission'
+                  and @method='post'
+                  and @base64RsaPublicKey='MIIB'
+                ]
+                """
+            ],
+        )
diff --git a/tests/validators/server.py b/tests/validators/server.py
index 3509cf773..8acb9ee78 100644
--- a/tests/validators/server.py
+++ b/tests/validators/server.py
@@ -2,6 +2,7 @@
 import os
 import posixpath
 import threading
+import time
 from http.server import SimpleHTTPRequestHandler
 from socketserver import ThreadingTCPServer
 from urllib.parse import unquote
@@ -70,13 +71,18 @@ def __init__(self, port=8000):
             self._server_address, self._handler, bind_and_activate=False
         )
 
-    def _bind_and_activate(self):
+    def _bind_and_activate(self, tries: int = 0):
+        tries += 1
         try:
             self.httpd.server_bind()
             self.httpd.server_activate()
-        except Exception as e:
+        except OSError as e:
             self.httpd.server_close()
-            raise e
+            if 5 < tries:
+                raise e
+            else:
+                time.sleep(0.5)
+                self._bind_and_activate(tries=tries)
 
     def start(self):
         self._bind_and_activate()