Ad
  • Custom User Avatar

    ok, just forget about the global TRACE thing, keep it at True in the tests.

    I'm not competent anymore for the other stuff, so just poke at me when the translation is ready for approval.

    NOTE: for an unknown reason, I don't receive any notification anymore when a message is posted here, so when done or if some context is needed plz poke at me on discord.

    Cheers

  • Custom User Avatar

    Note: you can use assert! with a custom comparison as first argument (e.g. compare actual to expected), and a fully custom assertion message as second argument. This doesn't output the boilerplate "left == right" stuff that you get with assert_eq!, only your own assertion message.

  • Custom User Avatar

    ok, code used in python:

            m = Molecule().brancher(1,5).bounder((2,2,5,2), (4,2,1,1)).mutate((1,1,'H'))
            m.brancher(3).bounder((2,3, 1,3), (2,3, 3,3))
            exp = ['Atom(H.1: C5)', 'Atom(C.2: C3)', 'Atom(C.3: C2,C4,C6)', 'Atom(C.4: C3,C5)', 'Atom(C.5: C4,C6,H)', 'Atom(C.6: C3,C5)', 'Atom(C.7: C8,C8)', 'Atom(C.8: C7,C7,C9,C9)', 'Atom(C.9: C8,C8)'] 
            test.assert_equals(extractNoneHToStr(m, True), exp, "Check correctness so far, before the tricky part")
            
            m.closer().unlock()
            test.expect_error( "Should remove any empty branch: m.add([2,2,'P']) shouldn't work, now, since the targeted carbon (previously thrid on the branch) cannot accept new bounds.",
                               lambda: m.add((2,2,"P")) )
            
            m.add((2,1,"P"))
            exp = ['Atom(C.1: C2)', 'Atom(C.2: C1,C3,C5,P9)', 'Atom(C.3: C2,C4)', 'Atom(C.4: C3,C5)', 'Atom(C.5: C2,C4)', 'Atom(C.6: C7,C7)', 'Atom(C.7: C6,C6,C8,C8)', 'Atom(C.8: C7,C7)', 'Atom(P.9: C2)']
            test.assert_equals(extractNoneHToStr(m), exp, "Should have removed correctly the first empty branch and should have used what was previously the second branch as the currently first one")
         
    
  • Custom User Avatar

    ok, sounds good to me.

    .1 => I still see some left/right, as in:

    tests::atom_spec::atom_display
    assertion failed: `(left == right)`
      left: `"Atom(C.1)"`,
     right: `"Atom(C.1x"`: "Atom(C.1)" should be "Atom(C.1x"
    

    Would that be possible to have actual/expected (or the opposite, I didn't check which one it is) instead of left/right everywhere?

    .3 => yeah, good idea. I'll update the other languages.

    .4 => about the TRACE thing, the idea is that the user can change the setting through their solution, to help debugging. I didn't see it defined in there or in preloaded => ?

    Everything else seems fine to me.

    Cheers!

  • Custom User Avatar

    'kay, I'll need to dig into this later. If you didn't get feedback in the next 48h, don't hesitate to poke at me again.

  • Custom User Avatar

    Ok, cool. I don't really know how I missed the random tests, but anyway...


    So, I've been poking at the translation in a fork panel and I have a couple of questions/demands:

    • The assertions are missing a clear feedback. Like:

      assertion failed: `(left == right)`
        left: `5`,
       right: `0`
       
      assertion failed: `(left == right)`
        left: `"CH4"`,
       right: `""`: Testing raw formula
      

      => what is left? what is right? (as in, actual vs expected) => Please add custom assertion messages to the assertions

      In the mod property_and_method_locking section, the assertion messages are there, but they are not giving all the information the python version is providing. Please

    • In some cases, the test function may fail on unexpected inputs. For example, returning an empty Vec from fn atoms(&self) -> Vec<Atom> is causing

      index out of bounds: the len is 0 but the index is 1 at src/lib.rs:...:...
      

      ...in the fixed tests, but works in the random tests with stuff like:

      assertion failed: `(left == right)`
        left: `["Atom(C.1: C2)", "Atom(C.2: C1,C3)", ..., "Atom(C.88: C87)"]`,
       right: `[]`
      

      => is it possible to make the fixed tests work whatever the Vec length is?

    • I don't really like the fact that some feedback is printed to the console instead of being shown in the tests::... thing, but I guess that's a compromise we should accept, for readability... Tho, I'd like you to change some of the mod names, so that it's easier to understand what is tested, and also easier to find the equivalent in python (which is my ref version, as you probably guessed already):

      • mod carbohydrates -> mod create_and_bound_carbohydrates
      • mod additive_mutations -> mod mutation_then_additions
      • mod basic_invalid_bonds -> mod basic_invalid_builds
      • I couldn't find the tests matching @test.describe("Spot correctly an invalid bound where the second atom bounded would exceed its valence number"), from the python version. => ?
      • mod valid_inputs_before_invalid -> mod molecule_integrity__keep_valids__stop_at_first_failure
      • mod property_and_method_locking -> mod properties_and_methods_vs_locking
      • fn desaturate() -> fn remove_hydrogens_and_update_id_numbers().
        Note: also, a typo in the isobutane name argument: isobutane 0 add 1 explicit H... -> isobutane - add 1 explicit H...
      • fn unlocking_after_h_mutation -> fn unlocking_after_mutation_to_h
      • I didn't see the @test.it("Remove empty branches, after unlocking") section at this point (if it's somewhere else, please keep the order of the section as in the python version so that I can compare them more easily)
      • fn all_empty_branches_after_unlock_errs -> fn EmptyMolecule_Err_if_no_branches_after_unlock.
        Also, typo: EmptyMoleculeecule -> EmptyMolecule (here and in other places)
    • In python, the option to TRACE the instructions in the random tests is optional. From what I guess of the tests, the current rust version is instead printing everything to the console, unconditionally. Is it possible to make it work like it does in python? If not or too much of a hassle, it's ok like this.

    • The random tests apparently generate a lot of calls to fonctions without data. Like - Bond: [], - Mutate: [], - Add: [], ... This never happens in python, so please enforce the presence of at least one element of input/data for every function.

    • I see this somewhere: m.bond(&[(5, 1, 3, 2)]).expect("The chain -C-Br added on the carbon [...]); which in python is like ...expect(False, "The chain -C-Br added on the carbon [...]). Are those both equivalent? (underlying question: is that an unconditionally failing test?)

    Sorry for the wall of text... 'x)

  • Custom User Avatar

    thanks! approved!