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

Update protein-translation tests #1115

Merged

Conversation

sjwarner-bp
Copy link
Contributor

Fixes #1103

@sjwarner-bp
Copy link
Contributor Author

I've just seen this error - can someone let me know if there is a maximum line size, as this seems to be the error (but I'd like to check first) 🙂

@N-Parsons
Copy link
Contributor

N-Parsons commented Nov 20, 2017

Hi @sjwarner-bp, if you click through to Travis-CI using the "details" link in the bottom box, it will take you to an overview of the tests. You can then pick one of the Python versions (usually doesn't matter which) and then it will show an output of what's wrong.

In your case, it looks like flake8 formatting errors (link:

$ flake8
./exercises/protein-translation/protein_translation_test.py:7:1: E302 expected 2 blank lines, found 1
./exercises/protein-translation/protein_translation_test.py:49:80: E501 line too long (80 > 79 characters)
./exercises/protein-translation/protein_translation_test.py:54:80: E501 line too long (82 > 79 characters)
./exercises/protein-translation/protein_translation_test.py:59:80: E501 line too long (83 > 79 characters)
./exercises/protein-translation/protein_translation_test.py:61:33: E231 missing whitespace after ','
./exercises/protein-translation/protein_translation_test.py:61:44: E231 missing whitespace after ','

You can also install flake8 locally using pip install flake8, and then run it with flake8 <path>.

@cmccandless
Copy link
Contributor

@N-Parsons (cc @sjwarner-bp )

You can also install flake8 locally using pip install flake8, and then run it with flake8 [path].

I think you accidentally dropped an "8". Also note that path is optional and defaults to "./" 😄

@sjwarner-bp
Copy link
Contributor Author

Perfect - thanks for letting me know guys, I've installed it locally and will remember to use it in future! 🙂

@@ -35,25 +37,34 @@ def test_identifies_stop_codons(self):
for codon in ['UAA', 'UAG', 'UGA']:
self.assertEqual('STOP', of_codon(codon))
Copy link
Contributor

Choose a reason for hiding this comment

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

Per discussion in exercism/problem-specifications#997, I don't think we should test for intermediate results. Thus, all tests should test of_rna() (proteins() in the spec).

@@ -35,25 +37,34 @@ def test_identifies_stop_codons(self):
for codon in ['UAA', 'UAG', 'UGA']:
self.assertEqual('STOP', of_codon(codon))

def test_translates_rna_strand_into_correct_protein(self):
def test_translates_rna_strand_into_correct_protein_list(self):
strand = 'AUGUUUUGG'
expected = ['Methionine', 'Phenylalanine', 'Tryptophan']
self.assertEqual(expected, of_rna(strand))
Copy link
Contributor

Choose a reason for hiding this comment

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

In an effort to maintain consistency with the spec, of_rna() should probably be renamed to proteins().

@sjwarner-bp
Copy link
Contributor Author

Hi @cmccandless - just to double check so I don't make the wrong edits, all of_codon() and of_rna() should be renamed to proteins()?

@cmccandless
Copy link
Contributor

@sjwarner-bp Correct, however note that the tests that currently use of_codon() will need to be modified to expect a list (albeit with only 1 element each).

@sjwarner-bp
Copy link
Contributor Author

Perfect, I understand you 😄

@sjwarner-bp
Copy link
Contributor Author

Sorry, I have just picked this up again after a hectic week! I'm trying to do this but running into trouble.

Just to clarify, the things I need to do to satisfy this is:

  • Change all reference of of_codon and of_strand to protein
  • Update the skeleton file with a single definition for protein and nothing else
  • Update the reference solution

So I have done the first two, but the final one is causing me a little confusion - do you have any guidance?

@cmccandless
Copy link
Contributor

@sjwarner-bp Have you tried simply renaming (in example.py) of_rna->protein, with no other changes?

@sjwarner-bp
Copy link
Contributor Author

Hi @cmccandless , I have renamed of_rna to protein and I end up with 13 failed tests, all of which have AssertationError <some_output> != None

@cmccandless
Copy link
Contributor

@sjwarner-bp Can you please drop the contents of your example.py and protein_translation_test.py in a multi-file gist so I can take a look?

@sjwarner-bp
Copy link
Contributor Author

Sure thing - here it is

@cmccandless
Copy link
Contributor

Don't forget to change

from protein_translation import protein

to

from example import protein

when running tests locally (revert before committing).

@sjwarner-bp
Copy link
Contributor Author

Yup, done that - I still end up with 8 failing tests for some reason though 😱. I've tried the (previously) of_codon with and without list notation around the parameter.

@cmccandless
Copy link
Contributor

Make sure the tests that were previously of_codon tests both pass in and expect a list.

@sjwarner-bp
Copy link
Contributor Author

Do you mean something like the following?

def test_AUG_translates_to_methionine(self):
        self.assertEqual(['Methionine'], protein(['AUG']))

I've tried this (and then without the [ ]) but it doesn't seem to work.

@cmccandless
Copy link
Contributor

Sorry; I wasn't paying full attention to this and I didn't take the time to fully understand the inputs/outputs.

According to the canonical data, here are a few notes:

  • Tests should only reference proteins() (I told you to call it protein, but should pluralize for consistency)
  • proteins() expected a string (my bad, told you to provide a list, but that's wrong)
  • proteins() returns a list, regardless of the number of elements in the input
  • If proteins() encounters a STOP codon, translation ceases and the results so far are returned. The STOP protein itself is not included.

See if any of that helps. I was able to make a few quick edits to your code to make it work, but you will benefit more from this if you figure it out yourself 😄

from protein_translation import of_codon, of_rna
from protein_translation import proteins

# Tests adapted from problem-specifications/canonical-data.json @ v1.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with other exercises, can you please change the spacing around this line? Should be 2 blank lines above, 1 below. (In the back of my mind I have a feeling I may have already said this, but switched the numbers; if so, I apologize for the misinformation)

@cmccandless cmccandless merged commit e78fd80 into exercism:master Dec 6, 2017
@cmccandless
Copy link
Contributor

@sjwarner-bp Merged. Thanks for working on this!

@sjwarner-bp
Copy link
Contributor Author

Thanks for helping me out!

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.

4 participants